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

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Amul Sul <sulamul(at)gmail(dot)com>
Cc: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Simon Riggs <simon(dot)riggs(at)enterprisedb(dot)com>, Greg Stark <stark(at)mit(dot)edu>, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>, Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Skip temporary table schema name from explain-verbose output.
Date: 2021-07-26 16:49:15
Message-ID: 160784.1627318155@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

I wrote:
> Simon Riggs <simon(dot)riggs(at)enterprisedb(dot)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

Attachment Content-Type Size
0001-Hide-internal-temp-schema-name-2.patch text/x-diff 7.1 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Simon Riggs 2021-07-26 17:01:19 Re: Skip temporary table schema name from explain-verbose output.
Previous Message Bossart, Nathan 2021-07-26 16:14:23 Re: .ready and .done files considered harmful