Re: Forbid to DROP temp tables of other sessions

On Mon, Mar 17, 2025 at 10:58 AM Daniil Davydov <3danissimo(at)gmail(dot)com>
wrote:

>
> Please see v4 patch (only comment change).
>
>
/*
- * For missing_ok, allow a non-existent schema name to
- * return InvalidOid.
+ * We don't allow users to access temp tables of other
+ * sessions (consider adding GUC for to allow to DROP such
+ * tables?).
*/

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.

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? The best you can do in the grammar is break
separation of concerns by programming to pg_temp as you are here, and
deduce it is temporary, or not, so long as a schema is listed.

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.

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.

I guess if others more knowledgeable agree existing assertions are OK this
patch is strictly improving things. Whether it is enough in the case of a
non-schema qualified relation in the query text I cannot say.

David J.