Thread: Skip temporary table schema name from explain-verbose output.
Hi,
Temporary tables usually gets a unique schema name, see this:
postgres=# create temp table foo(i int);
CREATE TABLE
postgres=# explain verbose select * from foo;
QUERY PLAN
-----------------------------------------------------------------
Seq Scan on pg_temp_3.foo (cost=0.00..35.50 rows=2550 width=4)
Output: i
(2 rows)
The problem is that explain-verbose regression test output becomes
unstable when several concurrently running tests operate on temporary
tables.
I was wondering can we simply skip the temporary schema name from the
explain-verbose output or place the "pg_temp" schema name?
Thoughts/Suggestions?
Regares,
Amul
On Tue, Apr 27, 2021 at 10:51 AM Amul Sul <sulamul@gmail.com> wrote:
>
> Hi,
>
> Temporary tables usually gets a unique schema name, see this:
>
> postgres=# create temp table foo(i int);
> CREATE TABLE
> postgres=# explain verbose select * from foo;
> QUERY PLAN
> -----------------------------------------------------------------
> Seq Scan on pg_temp_3.foo (cost=0.00..35.50 rows=2550 width=4)
> Output: i
> (2 rows)
>
> The problem is that explain-verbose regression test output becomes
> unstable when several concurrently running tests operate on temporary
> tables.
>
> I was wondering can we simply skip the temporary schema name from the
> explain-verbose output or place the "pg_temp" schema name?
>
> Thoughts/Suggestions?
How about using an explain filter to replace the unstable text
pg_temp_3 to pg_temp_N instead of changing it in the core? Following
are the existing explain filters: explain_filter,
explain_parallel_append, explain_analyze_without_memory,
explain_resultcache, explain_parallel_sort_stats, explain_sq_limit.
Looks like some of the test cases already replace pg_temp_nn with pg_temp:
-- \dx+ would expose a variable pg_temp_nn schema name, so we can't use it here
select regexp_replace(pg_describe_object(classid, objid, objsubid),
'pg_temp_\d+', 'pg_temp', 'g') as "Object description"
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
On Tue, Apr 27, 2021 at 11:07 AM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: > > On Tue, Apr 27, 2021 at 10:51 AM Amul Sul <sulamul@gmail.com> wrote: > > > > Hi, > > > > Temporary tables usually gets a unique schema name, see this: > > > > postgres=# create temp table foo(i int); > > CREATE TABLE > > postgres=# explain verbose select * from foo; > > QUERY PLAN > > ----------------------------------------------------------------- > > Seq Scan on pg_temp_3.foo (cost=0.00..35.50 rows=2550 width=4) > > Output: i > > (2 rows) > > > > The problem is that explain-verbose regression test output becomes > > unstable when several concurrently running tests operate on temporary > > tables. > > > > I was wondering can we simply skip the temporary schema name from the > > explain-verbose output or place the "pg_temp" schema name? > > > > Thoughts/Suggestions? > > How about using an explain filter to replace the unstable text > pg_temp_3 to pg_temp_N instead of changing it in the core? Following > are the existing explain filters: explain_filter, > explain_parallel_append, explain_analyze_without_memory, > explain_resultcache, explain_parallel_sort_stats, explain_sq_limit. > Well, yes eventually, that will be the kludge. I was wondering if that table is accessible in a query via pg_temp schema then why should bother about printing the pg_temp_N schema name which is an internal purpose. > Looks like some of the test cases already replace pg_temp_nn with pg_temp: > -- \dx+ would expose a variable pg_temp_nn schema name, so we can't use it here > select regexp_replace(pg_describe_object(classid, objid, objsubid), > 'pg_temp_\d+', 'pg_temp', 'g') as "Object description" > This \d could be one example of why not simply show pg_temp instead of pg_temp_N. Regards, Amul
On Tue, Apr 27, 2021 at 12:23 PM Amul Sul <sulamul@gmail.com> wrote: > > > > How about using an explain filter to replace the unstable text > > pg_temp_3 to pg_temp_N instead of changing it in the core? Following > > are the existing explain filters: explain_filter, > > explain_parallel_append, explain_analyze_without_memory, > > explain_resultcache, explain_parallel_sort_stats, explain_sq_limit. > > > > Well, yes eventually, that will be the kludge. I was wondering if that > table is accessible in a query via pg_temp schema then why should > bother about printing the pg_temp_N schema name which is an internal > purpose. Although only the associated session can access objects from that schema, I think, the entries in pg_class have different namespace oids and are accessible from other sessions. So knowing the actual schema name is useful for debugging purposes. Using auto_explain, the explain output goes to server log, where access to two temporary tables with the same name from different sessions can be identified by the actual schema name easily. I am not sure whether we should change explain output only for the sake of stable tests. You could add a flag to EXPLAIN to mask pg_temp name but that's probably an overkill. Filtering is a better option for tests. -- Best Wishes, Ashutosh Bapat
On Tue, Apr 27, 2021 at 6:59 PM Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote: > > On Tue, Apr 27, 2021 at 12:23 PM Amul Sul <sulamul@gmail.com> wrote: > > > > > > How about using an explain filter to replace the unstable text > > > pg_temp_3 to pg_temp_N instead of changing it in the core? Following > > > are the existing explain filters: explain_filter, > > > explain_parallel_append, explain_analyze_without_memory, > > > explain_resultcache, explain_parallel_sort_stats, explain_sq_limit. > > > > > > > Well, yes eventually, that will be the kludge. I was wondering if that > > table is accessible in a query via pg_temp schema then why should > > bother about printing the pg_temp_N schema name which is an internal > > purpose. > > Although only the associated session can access objects from that > schema, I think, the entries in pg_class have different namespace oids > and are accessible from other sessions. So knowing the actual schema > name is useful for debugging purposes. Using auto_explain, the explain > output goes to server log, where access to two temporary tables with > the same name from different sessions can be identified by the actual > schema name easily. > > I am not sure whether we should change explain output only for the > sake of stable tests. I agree to not change the explain code, just for tests. > You could add a flag to EXPLAIN to mask pg_temp name but that's > probably an overkill. IMO, you are right, it will be an overkill. We might end up having requests to add flags for other cases as well. > Filtering is a better option for tests. +1. EXPLAIN output filtering is not something new, we have already stabilized a few tests. With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
On Tue, Apr 27, 2021 at 7:08 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: > > On Tue, Apr 27, 2021 at 6:59 PM Ashutosh Bapat > <ashutosh.bapat.oss@gmail.com> wrote: > > > > On Tue, Apr 27, 2021 at 12:23 PM Amul Sul <sulamul@gmail.com> wrote: > > > > > > > > How about using an explain filter to replace the unstable text > > > > pg_temp_3 to pg_temp_N instead of changing it in the core? Following > > > > are the existing explain filters: explain_filter, > > > > explain_parallel_append, explain_analyze_without_memory, > > > > explain_resultcache, explain_parallel_sort_stats, explain_sq_limit. > > > > > > > > > > Well, yes eventually, that will be the kludge. I was wondering if that > > > table is accessible in a query via pg_temp schema then why should > > > bother about printing the pg_temp_N schema name which is an internal > > > purpose. > > > > Although only the associated session can access objects from that > > schema, I think, the entries in pg_class have different namespace oids > > and are accessible from other sessions. So knowing the actual schema > > name is useful for debugging purposes. Using auto_explain, the explain > > output goes to server log, where access to two temporary tables with > > the same name from different sessions can be identified by the actual > > schema name easily. > > Make sense, we would lose the ability to differentiate temporary tables from the auto_explain logs. Regards, Amul
> On Tue, Apr 27, 2021 at 7:08 PM Bharath Rupireddy > <bharath.rupireddyforpostgres@gmail > Make sense, we would lose the ability to differentiate temporary > tables from the auto_explain logs. There's no useful differentiation that can be done with the temp schema name. They're assigned on connection start randomly from the pool of temp schemas. The names you find in the log won't be useful and as new connections are made the same schema names will be reused for different connections. I would say it makes sense to remove them -- except perhaps it makes it harder to parse explain output. If explain verbose always includes the schema then it's easier for a parser to make sense of the explain plan output without having to be prepared to sometimes see a schema and sometimes not. That's probably a pretty hypothetical concern however since all the explain plan parsers that actually exist are prepared to deal with non-verbose plans anyways. And we have actual machine-readable formats too anyways. -- greg
Greg Stark <stark@mit.edu> writes:
>> On Tue, Apr 27, 2021 at 7:08 PM Bharath Rupireddy
>> <bharath.rupireddyforpostgres@gmail
>> Make sense, we would lose the ability to differentiate temporary
>> tables from the auto_explain logs.
> There's no useful differentiation that can be done with the temp
> schema name.
Agreed.
> I would say it makes sense to remove them -- except perhaps it makes
> it harder to parse explain output.
I don't think we should remove them. However, it could make sense to
print the "pg_temp" alias instead of the real schema name when we
are talking about myTempNamespace. Basically try to make that alias
a bit less leaky.
regards, tom lane
On Wed, Apr 28, 2021 at 7:56 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Greg Stark <stark@mit.edu> writes: > >> On Tue, Apr 27, 2021 at 7:08 PM Bharath Rupireddy > >> <bharath.rupireddyforpostgres@gmail > >> Make sense, we would lose the ability to differentiate temporary > >> tables from the auto_explain logs. > > > There's no useful differentiation that can be done with the temp > > schema name. > I see. > Agreed. > > > I would say it makes sense to remove them -- except perhaps it makes > > it harder to parse explain output. > > I don't think we should remove them. However, it could make sense to > print the "pg_temp" alias instead of the real schema name when we > are talking about myTempNamespace. Basically try to make that alias > a bit less leaky. +1, let's replace it by "pg_temp" -- did the same in that attached 0001 patch. Also, I am wondering if we need a similar kind of handling in psql '\d' meta-command as well? I did trial changes in the 0002 patch, but I am not very sure about it & a bit skeptical for code change as well. Do let me know if you have any suggestions/thoughts or if we don't want to, so please ignore that patch, thanks. Regards, Amul
Attachment
On Thu, 29 Apr 2021 at 08:17, Amul Sul <sulamul@gmail.com> wrote: > On Wed, Apr 28, 2021 at 7:56 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > I don't think we should remove them. However, it could make sense to > > print the "pg_temp" alias instead of the real schema name when we > > are talking about myTempNamespace. Basically try to make that alias > > a bit less leaky. > > +1, let's replace it by "pg_temp" -- did the same in that attached 0001 patch. Sounds like a good change. Surely we need a test to exercise this works? Otherwise ready... -- Simon Riggs http://www.EnterpriseDB.com/
Simon Riggs <simon.riggs@enterprisedb.com> writes:
> Sounds like a good change.
> Surely we need a test to exercise this works? Otherwise ready...
There are lots of places in the core regression tests where we'd have
used a temp table, except that we needed to do EXPLAIN and the results
would've been unstable, so we used a short-lived plain table instead.
Find one of those and change it to use a temp table.
regards, tom lane
I wrote:
> Simon Riggs <simon.riggs@enterprisedb.com> writes:
>> Surely we need a test to exercise this works? Otherwise ready...
> There are lots of places in the core regression tests where we'd have
> used a temp table, except that we needed to do EXPLAIN and the results
> would've been unstable, so we used a short-lived plain table instead.
> Find one of those and change it to use a temp table.
Hmm ... I looked through the core regression tests' usages of EXPLAIN
VERBOSE and didn't really find any that it seemed to make sense to change
that way. I guess we've been more effective at programming around that
restriction than I thought.
Anyway, after looking at the 0001 patch, I think there's a pretty large
oversight in that it doesn't touch ruleutils.c, although EXPLAIN relies
heavily on that to print expressions and suchlike. We could account
for that as in the attached revision of 0001.
However, I wonder whether this isn't going in the wrong direction.
Instead of piecemeal s/get_namespace_name/get_namespace_name_or_temp/,
we should consider just putting this behavior right into
get_namespace_name, and dropping the separate get_namespace_name_or_temp
function. I can't really see any situation in which it's important
to report the exact schema name of our own temp schema.
On the other hand, I don't like 0002 one bit, because it's not accounting
for whether the temp schema it's mangling is *our own* temp schema or some
other session's. I do not think it is wise or even safe to report some
other temp schema as being "pg_temp". By the same token, I wonder whether
this bit in event_trigger.c is a good idea or a safety hazard:
/* XXX not quite get_namespace_name_or_temp */
if (isAnyTempNamespace(schema_oid))
schema = pstrdup("pg_temp");
else
schema = get_namespace_name(schema_oid);
Alvaro, you seem to be responsible for both the existence of the separate
get_namespace_name_or_temp function and the fact that it's being avoided
here. I wonder what you think about this.
regards, tom lane
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index f15c97ad7a..51fac77f3d 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -2854,7 +2854,7 @@ postgresExplainForeignScan(ForeignScanState *node, ExplainState *es)
{
char *namespace;
- namespace = get_namespace_name(get_rel_namespace(rte->relid));
+ namespace = get_namespace_name_or_temp(get_rel_namespace(rte->relid));
appendStringInfo(relations, "%s.%s",
quote_identifier(namespace),
quote_identifier(relname));
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 340db2bac4..36fbe129cf 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -3747,7 +3747,7 @@ ExplainTargetRel(Plan *plan, Index rti, ExplainState *es)
Assert(rte->rtekind == RTE_RELATION);
objectname = get_rel_name(rte->relid);
if (es->verbose)
- namespace = get_namespace_name(get_rel_namespace(rte->relid));
+ namespace = get_namespace_name_or_temp(get_rel_namespace(rte->relid));
objecttag = "Relation Name";
break;
case T_FunctionScan:
@@ -3774,8 +3774,7 @@ ExplainTargetRel(Plan *plan, Index rti, ExplainState *es)
objectname = get_func_name(funcid);
if (es->verbose)
- namespace =
- get_namespace_name(get_func_namespace(funcid));
+ namespace = get_namespace_name_or_temp(get_func_namespace(funcid));
}
}
objecttag = "Function Name";
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 5e7108f903..4df8cc5abf 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -1617,7 +1617,7 @@ pg_get_statisticsobj_worker(Oid statextid, bool columns_only, bool missing_ok)
if (!columns_only)
{
- nsp = get_namespace_name(statextrec->stxnamespace);
+ nsp = get_namespace_name_or_temp(statextrec->stxnamespace);
appendStringInfo(&buf, "CREATE STATISTICS %s",
quote_qualified_identifier(nsp,
NameStr(statextrec->stxname)));
@@ -2811,7 +2811,7 @@ pg_get_functiondef(PG_FUNCTION_ARGS)
* We always qualify the function name, to ensure the right function gets
* replaced.
*/
- nsp = get_namespace_name(proc->pronamespace);
+ nsp = get_namespace_name_or_temp(proc->pronamespace);
appendStringInfo(&buf, "CREATE OR REPLACE %s %s(",
isfunction ? "FUNCTION" : "PROCEDURE",
quote_qualified_identifier(nsp, name));
@@ -11183,7 +11183,7 @@ get_opclass_name(Oid opclass, Oid actual_datatype,
appendStringInfo(buf, " %s", quote_identifier(opcname));
else
{
- nspname = get_namespace_name(opcrec->opcnamespace);
+ nspname = get_namespace_name_or_temp(opcrec->opcnamespace);
appendStringInfo(buf, " %s.%s",
quote_identifier(nspname),
quote_identifier(opcname));
@@ -11495,7 +11495,7 @@ generate_relation_name(Oid relid, List *namespaces)
need_qual = !RelationIsVisible(relid);
if (need_qual)
- nspname = get_namespace_name(reltup->relnamespace);
+ nspname = get_namespace_name_or_temp(reltup->relnamespace);
else
nspname = NULL;
@@ -11527,7 +11527,7 @@ generate_qualified_relation_name(Oid relid)
reltup = (Form_pg_class) GETSTRUCT(tp);
relname = NameStr(reltup->relname);
- nspname = get_namespace_name(reltup->relnamespace);
+ nspname = get_namespace_name_or_temp(reltup->relnamespace);
if (!nspname)
elog(ERROR, "cache lookup failed for namespace %u",
reltup->relnamespace);
@@ -11639,7 +11639,7 @@ generate_function_name(Oid funcid, int nargs, List *argnames, Oid *argtypes,
p_funcid == funcid)
nspname = NULL;
else
- nspname = get_namespace_name(procform->pronamespace);
+ nspname = get_namespace_name_or_temp(procform->pronamespace);
result = quote_qualified_identifier(nspname, proname);
@@ -11702,7 +11702,7 @@ generate_operator_name(Oid operid, Oid arg1, Oid arg2)
nspname = NULL;
else
{
- nspname = get_namespace_name(operform->oprnamespace);
+ nspname = get_namespace_name_or_temp(operform->oprnamespace);
appendStringInfo(&buf, "OPERATOR(%s.", quote_identifier(nspname));
}
@@ -11790,7 +11790,7 @@ add_cast_to(StringInfo buf, Oid typid)
typform = (Form_pg_type) GETSTRUCT(typetup);
typname = NameStr(typform->typname);
- nspname = get_namespace_name(typform->typnamespace);
+ nspname = get_namespace_name_or_temp(typform->typnamespace);
appendStringInfo(buf, "::%s.%s",
quote_identifier(nspname), quote_identifier(typname));
@@ -11822,7 +11822,7 @@ generate_qualified_type_name(Oid typid)
typtup = (Form_pg_type) GETSTRUCT(tp);
typname = NameStr(typtup->typname);
- nspname = get_namespace_name(typtup->typnamespace);
+ nspname = get_namespace_name_or_temp(typtup->typnamespace);
if (!nspname)
elog(ERROR, "cache lookup failed for namespace %u",
typtup->typnamespace);
@@ -11856,7 +11856,7 @@ generate_collation_name(Oid collid)
collname = NameStr(colltup->collname);
if (!CollationIsVisible(collid))
- nspname = get_namespace_name(colltup->collnamespace);
+ nspname = get_namespace_name_or_temp(colltup->collnamespace);
else
nspname = NULL;
diff --git a/src/backend/utils/cache/lsyscache.c b/src/backend/utils/cache/lsyscache.c
index 6bba5f8ec4..4ebaa552a2 100644
--- a/src/backend/utils/cache/lsyscache.c
+++ b/src/backend/utils/cache/lsyscache.c
@@ -3340,7 +3340,7 @@ char *
get_namespace_name_or_temp(Oid nspid)
{
if (isTempNamespace(nspid))
- return "pg_temp";
+ return pstrdup("pg_temp");
else
return get_namespace_name(nspid);
}
diff --git a/src/test/regress/expected/explain.out b/src/test/regress/expected/explain.out
index cda28098ba..16e196a7bd 100644
--- a/src/test/regress/expected/explain.out
+++ b/src/test/regress/expected/explain.out
@@ -477,6 +477,19 @@ select jsonb_pretty(
(1 row)
rollback;
+-- Test display of temporary objects
+create temp table t1(f1 float8);
+create function pg_temp.mysin(float8) returns float8 language plpgsql
+as 'begin return sin($1); end';
+select explain_filter('explain (verbose) select * from t1 where pg_temp.mysin(f1) < 0.5');
+ explain_filter
+------------------------------------------------------------
+ Seq Scan on pg_temp.t1 (cost=N.N..N.N rows=N width=N)
+ Output: f1
+ Filter: (pg_temp.mysin(t1.f1) < 'N.N'::double precision)
+(3 rows)
+
+-- Test compute_query_id
set compute_query_id = on;
select explain_filter('explain (verbose) select * from int8_tbl i8');
explain_filter
diff --git a/src/test/regress/sql/explain.sql b/src/test/regress/sql/explain.sql
index 3f9ae9843a..f401d99409 100644
--- a/src/test/regress/sql/explain.sql
+++ b/src/test/regress/sql/explain.sql
@@ -104,5 +104,14 @@ select jsonb_pretty(
rollback;
+-- Test display of temporary objects
+create temp table t1(f1 float8);
+
+create function pg_temp.mysin(float8) returns float8 language plpgsql
+as 'begin return sin($1); end';
+
+select explain_filter('explain (verbose) select * from t1 where pg_temp.mysin(f1) < 0.5');
+
+-- Test compute_query_id
set compute_query_id = on;
select explain_filter('explain (verbose) select * from int8_tbl i8');
On Mon, 26 Jul 2021 at 17:49, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > I wrote: > > Simon Riggs <simon.riggs@enterprisedb.com> writes: > >> Surely we need a test to exercise this works? Otherwise ready... > > > There are lots of places in the core regression tests where we'd have > > used a temp table, except that we needed to do EXPLAIN and the results > > would've been unstable, so we used a short-lived plain table instead. > > Find one of those and change it to use a temp table. > > Hmm ... I looked through the core regression tests' usages of EXPLAIN > VERBOSE and didn't really find any that it seemed to make sense to change > that way. I guess we've been more effective at programming around that > restriction than I thought. > > Anyway, after looking at the 0001 patch, I think there's a pretty large > oversight in that it doesn't touch ruleutils.c, although EXPLAIN relies > heavily on that to print expressions and suchlike. We could account > for that as in the attached revision of 0001. > > However, I wonder whether this isn't going in the wrong direction. > Instead of piecemeal s/get_namespace_name/get_namespace_name_or_temp/, > we should consider just putting this behavior right into > get_namespace_name, and dropping the separate get_namespace_name_or_temp > function. I can't really see any situation in which it's important > to report the exact schema name of our own temp schema. That sounds much better because any wholesale change like that will affect 100s of places in extensions and it would be easier if we made just one change in core. -- Simon Riggs http://www.EnterpriseDB.com/
On Mon, Jul 26, 2021 at 12:49 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > I can't really see any situation in which it's important > to report the exact schema name of our own temp schema. It would actually be nice if there were some easy way of getting that for the rare situations in which there are problems. For example, if the catalog entries get corrupted and you can't access some table in your pg_temp schema, you might like to know which pg_temp schema you've got so that you can be sure to examine the right catalog entries to fix the problem or understand the problem or whatever you are trying to do. I don't much care exactly how we go about making that information available and I agree that showing pg_temp_NNN in EXPLAIN output is worse than just pg_temp. I'm just saying that concealing too thoroughly what is actually happening can be a problem in the rare instance where troubleshooting is required. -- Robert Haas EDB: http://www.enterprisedb.com
On 2021-Jul-26, Tom Lane wrote: > Alvaro, you seem to be responsible for both the existence of the separate > get_namespace_name_or_temp function and the fact that it's being avoided > here. I wonder what you think about this. The reason I didn't touch get_namespace_name then (e9a077cad379) was that I didn't want to change the user-visible behavior for any existing features; I was just after a way to implement dropped-object DDL trigger tracking. If we agree that displaying pg_temp instead of pg_temp_XXX everywhere is an improvement, then I don't see a reason not to change how get_namespace_name works and get rid of get_namespace_name_or_temp. I don't see much usefulness in displaying the exact name of the temp namespace anywhere, particularly since using "pg_temp" as a qualification in queries already refers to the current backend's temp namespace. Trying to refer to it by exact name in SQL may lead to affecting some other backend's temp objects ... -- Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/
Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Jul 26, 2021 at 12:49 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I can't really see any situation in which it's important
>> to report the exact schema name of our own temp schema.
> It would actually be nice if there were some easy way of getting that
> for the rare situations in which there are problems.
I experimented with pushing the behavior into get_namespace_name,
and immediately ran into problems, for example
--- /home/postgres/pgsql/src/test/regress/expected/jsonb.out 2021-03-01 16:32
:13.348655633 -0500
+++ /home/postgres/pgsql/src/test/regress/results/jsonb.out 2021-07-26 13:10
:53.523540855 -0400
@@ -320,11 +320,9 @@
where tablename = 'rows' and
schemaname = pg_my_temp_schema()::regnamespace::text
order by 1;
- attname | histogram_bounds
----------+--------------------------
- x | [1, 2, 3]
- y | ["txt1", "txt2", "txt3"]
-(2 rows)
+ attname | histogram_bounds
+---------+------------------
+(0 rows)
-- to_jsonb, timestamps
select to_jsonb(timestamp '2014-05-28 12:22:35.614298');
What's happening here is that regnamespace_out is returning
'pg_temp' which doesn't match any name visible in pg_namespace.
So that would pretty clearly break user queries as well as
our own tests. I'm afraid that the wholesale behavior change
I was imagining isn't going to work. Probably we'd better stick
to doing something close to the v2 patch I posted.
I'm still suspicious of that logic in event_trigger.c, though.
regards, tom lane
On 2021-Jul-26, Tom Lane wrote:
> On the other hand, I don't like 0002 one bit, because it's not accounting
> for whether the temp schema it's mangling is *our own* temp schema or some
> other session's. I do not think it is wise or even safe to report some
> other temp schema as being "pg_temp". By the same token, I wonder whether
> this bit in event_trigger.c is a good idea or a safety hazard:
>
> /* XXX not quite get_namespace_name_or_temp */
> if (isAnyTempNamespace(schema_oid))
> schema = pstrdup("pg_temp");
> else
> schema = get_namespace_name(schema_oid);
Oh, you meant this one. To be honest I don't remember *why* this code
wants to show remote temp tables as just "pg_temp" ... it's possible
that some test in the DDL-to-JSON code depended on this behavior.
Without spending too much time analyzing it, I agree that it seems
dangerous and might lead to referring to unintended objects. (Really,
my memory is not clear on *why* we would be referring to temp tables of
other sessions.)
--
Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/
"No necesitamos banderas
No reconocemos fronteras" (Jorge González)
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Oh, you meant this one. To be honest I don't remember *why* this code
> wants to show remote temp tables as just "pg_temp" ... it's possible
> that some test in the DDL-to-JSON code depended on this behavior.
> Without spending too much time analyzing it, I agree that it seems
> dangerous and might lead to referring to unintended objects. (Really,
> my memory is not clear on *why* we would be referring to temp tables of
> other sessions.)
Yeah, it's not very clear why that would happen, but if it does,
showing "pg_temp" seems pretty misleading. I tried replacing the
code with just get_namespace_name_or_temp(), and it still gets
through check-world, for whatever that's worth.
I'm inclined to change this in HEAD but leave it alone in the back
branches. While it seems pretty bogus, it's not clear if anyone
out there could be relying on the current behavior.
regards, tom lane
I wrote:
> I'm inclined to change this in HEAD but leave it alone in the back
> branches. While it seems pretty bogus, it's not clear if anyone
> out there could be relying on the current behavior.
I've pushed both the 0001 v2 patch and the event trigger change,
and am going to mark the CF entry closed, because leaving it open
would confuse the cfbot. I think there may still be room to do
something about pg_temp_NNN output in psql's \d commands as 0002
attempted to, but I don't have immediate ideas about how to do
that in a safe/clean way.
regards, tom lane