Hi,
On Tue, Mar 18, 2025 at 2:36 AM David G. Johnston
<david.g.johns...@gmail.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.g.johns...@gmail.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
From 137a768bdddd94bbe29a03ca8e527b9b483da5d8 Mon Sep 17 00:00:00 2001
From: Daniil Davidov <d.davy...@postgrespro.ru>
Date: Mon, 17 Mar 2025 15:43:15 +0700
Subject: [PATCH v5] Fix accessing other sessions temp tables
---
src/backend/catalog/namespace.c | 56 ++++++++++++++++++++------------
src/backend/commands/tablecmds.c | 3 +-
src/backend/nodes/makefuncs.c | 6 +++-
src/backend/parser/gram.y | 11 ++++++-
src/include/catalog/namespace.h | 2 ++
5 files changed, 55 insertions(+), 23 deletions(-)
diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index d97d632a7ef..d231030fb1d 100644
--- a/src/backend/catalog/namespace.c
+++ b/src/backend/catalog/namespace.c
@@ -499,28 +499,44 @@ RangeVarGetRelidExtended(const RangeVar *relation, LOCKMODE lockmode,
*/
if (relation->relpersistence == RELPERSISTENCE_TEMP)
{
- if (!OidIsValid(myTempNamespace))
- relId = InvalidOid; /* this probably can't happen? */
- else
- {
- if (relation->schemaname)
- {
- Oid namespaceId;
+ Oid namespaceId;
- namespaceId = LookupExplicitNamespace(relation->schemaname, missing_ok);
+ if (relation->schemaname)
+ {
+ namespaceId = LookupExplicitNamespace(relation->schemaname, missing_ok);
+ /*
+ * If the user has specified an existing temporary schema
+ * owned by another user.
+ */
+ if (OidIsValid(namespaceId) && namespaceId != myTempNamespace)
+ {
/*
- * For missing_ok, allow a non-existent schema name to
- * return InvalidOid.
+ * We don't allow users to access temp tables of other
+ * sessions except for the case of dropping tables.
*/
- if (namespaceId != myTempNamespace)
+ if (!(flags & RVR_OTHER_TEMP_OK))
ereport(ERROR,
- (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
- errmsg("temporary tables cannot specify a schema name")));
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("could not access temporary relations of other sessions")));
}
+ }
+ else
+ {
+ namespaceId = myTempNamespace;
- relId = get_relname_relid(relation->relname, myTempNamespace);
+ /*
+ * If this table was recognized as temporary, it means that we
+ * found it because backend's temporary namespace was specified
+ * in search_path. Thus, MyTempNamespace must contain valid oid.
+ */
+ Assert(OidIsValid(namespaceId));
}
+
+ if (missing_ok && !OidIsValid(namespaceId))
+ relId = InvalidOid;
+ else
+ relId = get_relname_relid(relation->relname, namespaceId);
}
else if (relation->schemaname)
{
@@ -3553,21 +3569,19 @@ get_namespace_oid(const char *nspname, bool missing_ok)
RangeVar *
makeRangeVarFromNameList(const List *names)
{
- RangeVar *rel = makeRangeVar(NULL, NULL, -1);
+ RangeVar *rel;
switch (list_length(names))
{
case 1:
- rel->relname = strVal(linitial(names));
+ rel = makeRangeVar(NULL, strVal(linitial(names)), -1);
break;
case 2:
- rel->schemaname = strVal(linitial(names));
- rel->relname = strVal(lsecond(names));
+ rel = makeRangeVar(strVal(linitial(names)), strVal(lsecond(names)), -1);
break;
case 3:
+ rel = makeRangeVar(strVal(lsecond(names)), strVal(lthird(names)), -1);
rel->catalogname = strVal(linitial(names));
- rel->schemaname = strVal(lsecond(names));
- rel->relname = strVal(lthird(names));
break;
default:
ereport(ERROR,
@@ -3774,6 +3788,8 @@ GetTempNamespaceProcNumber(Oid namespaceId)
return INVALID_PROC_NUMBER; /* no such namespace? */
if (strncmp(nspname, "pg_temp_", 8) == 0)
result = atoi(nspname + 8);
+ else if (strcmp(nspname, "pg_temp") == 0)
+ result = MyProcNumber;
else if (strncmp(nspname, "pg_toast_temp_", 14) == 0)
result = atoi(nspname + 14);
else
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 129c97fdf28..343293ef56e 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1594,7 +1594,8 @@ RemoveRelations(DropStmt *drop)
state.heapOid = InvalidOid;
state.partParentOid = InvalidOid;
- relOid = RangeVarGetRelidExtended(rel, lockmode, RVR_MISSING_OK,
+ relOid = RangeVarGetRelidExtended(rel, lockmode,
+ RVR_MISSING_OK | RVR_OTHER_TEMP_OK,
RangeVarCallbackForDropRelation,
&state);
diff --git a/src/backend/nodes/makefuncs.c b/src/backend/nodes/makefuncs.c
index e2d9e9be41a..62edf24b5c2 100644
--- a/src/backend/nodes/makefuncs.c
+++ b/src/backend/nodes/makefuncs.c
@@ -478,10 +478,14 @@ makeRangeVar(char *schemaname, char *relname, int location)
r->schemaname = schemaname;
r->relname = relname;
r->inh = true;
- r->relpersistence = RELPERSISTENCE_PERMANENT;
r->alias = NULL;
r->location = location;
+ if (r->schemaname && strncmp(r->schemaname, "pg_temp", 7) == 0)
+ r->relpersistence = RELPERSISTENCE_TEMP;
+ else
+ r->relpersistence = RELPERSISTENCE_PERMANENT;
+
return r;
}
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;
return r;
@@ -19464,6 +19468,11 @@ makeRangeVarFromQualifiedName(char *name, List *namelist, int location,
break;
}
+ if (r->schemaname && strncmp(r->schemaname, "pg_temp", 7) == 0)
+ r->relpersistence = RELPERSISTENCE_TEMP;
+ else
+ r->relpersistence = RELPERSISTENCE_PERMANENT;
+
return r;
}
diff --git a/src/include/catalog/namespace.h b/src/include/catalog/namespace.h
index 8c7ccc69a3c..9c45a30516e 100644
--- a/src/include/catalog/namespace.h
+++ b/src/include/catalog/namespace.h
@@ -72,6 +72,8 @@ typedef enum RVROption
RVR_MISSING_OK = 1 << 0, /* don't error if relation doesn't exist */
RVR_NOWAIT = 1 << 1, /* error if relation cannot be locked */
RVR_SKIP_LOCKED = 1 << 2, /* skip if relation cannot be locked */
+ RVR_OTHER_TEMP_OK = 1 << 3 /* don't error if relation is temp relation of
+ other session (needed for DROP command) */
} RVROption;
typedef void (*RangeVarGetRelidCallback) (const RangeVar *relation, Oid relId,
--
2.43.0
