Re: Forbid to DROP temp tables of other sessions - Mailing list pgsql-hackers

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.