Horton

Thread

Commits

  1. In event triggers, use "pg_temp" only for our own temp schema.

  2. Use the "pg_temp" schema alias in EXPLAIN and related output.

  1. Skip temporary table schema name from explain-verbose output.

    Amul Sul <sulamul@gmail.com> — 2021-04-27T05:20:22Z

    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

  2. Re: Skip temporary table schema name from explain-verbose output.

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-04-27T05:37:18Z

    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

  3. Re: Skip temporary table schema name from explain-verbose output.

    Amul Sul <sulamul@gmail.com> — 2021-04-27T06:52:20Z

    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

  4. Re: Skip temporary table schema name from explain-verbose output.

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2021-04-27T13:29:09Z

    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

  5. Re: Skip temporary table schema name from explain-verbose output.

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-04-27T13:38:19Z

    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

  6. Re: Skip temporary table schema name from explain-verbose output.

    Amul Sul <sulamul@gmail.com> — 2021-04-28T03:53:00Z

    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

  7. Re: Skip temporary table schema name from explain-verbose output.

    Greg Stark <stark@mit.edu> — 2021-04-28T14:16:35Z

    > 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

  8. Re: Skip temporary table schema name from explain-verbose output.

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-28T14:26:07Z

    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

  9. Re: Skip temporary table schema name from explain-verbose output.

    Amul Sul <sulamul@gmail.com> — 2021-04-29T07:16:43Z

    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

  10. Re: Skip temporary table schema name from explain-verbose output.

    Simon Riggs <simon.riggs@enterprisedb.com> — 2021-07-26T12:53:20Z

    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/

  11. Re: Skip temporary table schema name from explain-verbose output.

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-07-26T14:21:45Z

    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

  12. Re: Skip temporary table schema name from explain-verbose output.

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-07-26T16:49:15Z

    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

  13. Re: Skip temporary table schema name from explain-verbose output.

    Simon Riggs <simon.riggs@enterprisedb.com> — 2021-07-26T17:01:19Z

    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/

  14. Re: Skip temporary table schema name from explain-verbose output.

    Robert Haas <robertmhaas@gmail.com> — 2021-07-26T17:15:22Z

    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

  15. Re: Skip temporary table schema name from explain-verbose output.

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2021-07-26T17:30:42Z

    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/

  16. Re: Skip temporary table schema name from explain-verbose output.

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-07-26T17:33:29Z

    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

  17. Re: Skip temporary table schema name from explain-verbose output.

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2021-07-26T17:53:51Z

    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)

  18. Re: Skip temporary table schema name from explain-verbose output.

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-07-26T18:30:05Z

    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

  19. Re: Skip temporary table schema name from explain-verbose output.

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-07-27T16:12:12Z

    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