Re: Forbid to DROP temp tables of other sessions

Hi,

On Tue, Mar 18, 2025 at 2:36 AM David G. Johnston
<david(dot)g(dot)johnston(at)gmail(dot)com> wrote:
>
> "I want to give a better error message" is not a good enough reason to change this long-standing behavior in a back-patchable bug fix.
>.... and I do not agree that it is an improvement worth making in HEAD.

OK, In this case I'd rather agree.

On Tue, Mar 18, 2025 at 3:30 AM David G. Johnston
<david(dot)g(dot)johnston(at)gmail(dot)com> wrote:
>
> I sound like a broken record but this is a bug fix; introducing a GUC and changing unrelated behaviors is not acceptable in a back-patch.
>
> Superusers have been able to drop the temporary tables of other sessions for a long time - now is not the place to change that behavior.

Well, we can keep this option for the superuser, but we need to
explicitly indicate that we are inside the DROP operation.
Please see v5 patch. It is a bit experimental :
1) The original behavior in the LookupExplicitNamespace function has
been returned.
2) New RVROption has been added to indicate that we are inside the
DROP operation. Thus, superuser can drop other temp tables.

>
> Separately:
>
> Is the general logic here that we assume r->relpersistence is RELPERSISTENCE_PERMANENT until and unless we can prove it to be RELPERSISTENCE_TEMP?
>
> I'm trying to figure out whether there is still an issue when dealing with an unqualified name that would be found in the temporary schema.
> We've obviously already marked it permanent since we didn't have pg_temp in the query to inform us otherwise.
> But if we are changing permanent to temporary later because of search_path resolution then why did we have to get it right in the case where pg_temp is specified?
> I question whether "persistence" is something that gram.y should be dealing with at all. Shouldn't that property be solely determined in post-parse analysis via catalog lookup?

Hm, let's dive into this question. Why do I consider the original
behavior to be incorrect?

1)
If schema is not specified, then gram.y marks the table as PERMANENT.
1.1)
If we are looking for our temporary table, then "pg_temp_N" is present
in search_path, and RelnameGetRelid will return us this table.
1.2)
If we are looking for other temporary table, then RelnameGetRelid will
return `InvalidOid` and we will get a "relation not found" error.

2)
If schema is specified, then gram.y ALSO marks the table as PERMANENT.
2.1)
If we are looking for our temporary table, then
LookupExplicitNamespace will return us MyTempNamespace and we can get
our table without search_path lookup.
2.2)
If we are looking for other temporary table, then
LookupExplicitNamespace will return some valid oid, and we can get
other temp table without search_path lookup. Then, we will perform any
specified operation, assuming that we are working with a PERMANENT
table. This is a bug.

Let's summarize. If schema is not specified, we can safely mark the
table as PERMANENT, because we always can get the table from
search_path (and if the schema is not specified, then it is obvious
that the user wants to refer specifically to his table).
BUT, if schema is specified, we must know that the given relation is
TEMP before calling RangeVarGetRelidExtended (otherwise, there will be
an error, which I wrote about in paragraph 2.2).

>
> IOW, the original code here seems incorrect if this is the definitive place to determine relpersistence. in "case 1:", where there is no schema name, one cannot deduce relpersistence and it should remain NULL, IMO (not knowing what that might break...). The code this patch replaces is wrong for the same reason.

I hope that I managed to clarify this issue. As far as "pg_temp_"
prefix is reserved by the postgres kernel, we can definitely say that
we have encountered a temporary table when we see this prefix. IMO
there is no problem, that gram.y will do it.

>
> diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
> index 271ae26cbaf..f68948d475c 100644
> --- a/src/backend/parser/gram.y
> +++ b/src/backend/parser/gram.y
> @@ -19424,7 +19424,11 @@ makeRangeVarFromAnyName(List *names, int position, core_yyscan_t yyscanner)
> break;
> }
>
> - r->relpersistence = RELPERSISTENCE_PERMANENT;
> + if (r->schemaname && strncmp(r->schemaname, "pg_temp", 7) == 0)
> + r->relpersistence = RELPERSISTENCE_TEMP;
> + else
> + r->relpersistence = RELPERSISTENCE_PERMANENT;
> +
> r->location = position;
>
> The qualified name variant is fine since r->schemaname must be present by definition. Passing through the same if block, once with schemaname null (in makeRangeVar) and once with it populated (end of makeRangeVarFromQualifiedName) is a bit annoying.
>
> makeRangeVar has the same problem, assuming permanent when the schemaname argument is null.

Thank you for noticing it. I suggest we first confirm that the logic
(with persistence definition) remains in gram.y and then fix this
problem.

--
Best regards,
Daniil Davydov