Re: Bogus EXPLAIN results with column aliases for mismatched partitions

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com>
Cc: PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Bogus EXPLAIN results with column aliases for mismatched partitions
Date: 2019-12-02 21:45:18
Message-ID: 8343.1575323118@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

I wrote:
> Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com> writes:
>> On Mon, Dec 2, 2019 at 6:34 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> BTW, the existing code always schema-qualifies the relation names,
>>> on the rather lame grounds that it's producing the string without
>>> knowing whether EXPLAIN VERBOSE will be specified. In this code,
>>> the verbose flag is available so it would be trivial to make the
>>> output conform to EXPLAIN's normal policy. I didn't change that
>>> here because there'd be a bunch more test output diffs of no
>>> intellectual interest. Should we change it, or leave well enough
>>> alone?

>> I think it would be better to keep that as-is because otherwise, in
>> case of a foreign join or aggregate, EXPLAIN without the VERBOSE
>> option won't show any information about foreign tables involved in
>> that foreign join or aggregate, which isn't useful for users.

> No, I'm just talking about dropping the schema-qualification of table
> names when !es->verbose, not removing the Relations: output altogether.
> That would be more consistent with the rest of EXPLAIN's output.

Concretely, I'm thinking of the attached (on top of the other patch,
which I just pushed). This agrees exactly with what ExplainTargetRel
does for regular scans.

One could make an argument that we should schema-qualify, regardless
of the "verbose" flag, if the output format isn't EXPLAIN_FORMAT_TEXT.
That would reduce the amount of variability that plan analysis tools
have to cope with. However, ExplainTargetRel itself doesn't provide
the schema in non-verbose mode. Maybe it should, ie we should change
it like

case T_ModifyTable:
/* Assert it's on a real relation */
Assert(rte->rtekind == RTE_RELATION);
objectname = get_rel_name(rte->relid);
- if (es->verbose)
+ if (es->verbose || es->format != EXPLAIN_FORMAT_TEXT)
namespace = get_namespace_name(get_rel_namespace(rte->relid));
objecttag = "Relation Name";
break;

(and likewise for function schema names, a bit further down)?

regards, tom lane

Attachment Content-Type Size
further-sync-postgres_fdw-with-regular-EXPLAIN.patch text/x-diff 5.2 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tomas Vondra 2019-12-02 22:39:33 Re: surprisingly expensive join planning query
Previous Message Masahiko Sawada 2019-12-02 21:14:58 Re: Using XLogFileNameP in critical section