Re: Bogus EXPLAIN results with column aliases for mismatched partitions

On Mon, Dec 2, 2019 at 6:34 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I wrote:
> > Now, there is another thing that set_rtable_names() is doing that
> > postgres_fdw doesn't do, which is to unique-ify the chosen alias
> > names by adding "_NN" if the querytree contains multiple uses of
> > the same table alias. I don't see any practical way for postgres_fdw
> > to match that behavior, since it lacks global information about the
> > query. If we wanted to fix it, what we'd likely need to do is
> > postpone creation of the relation_name strings until EXPLAIN,
> > providing some way for postgres_fdw to ask ruleutils.c what alias
> > it'd assigned to a particular RTE.
>
> Hmmm ... so actually, that isn't *quite* as bad as I thought:
> ExplainState does already expose that information, so we just need
> to rearrange how postgres_fdw does things. Here's a revised proposed
> patch, which exposes (and fixes) several additional test cases where
> the Relations: string was previously visibly out of sync with what
> ruleutils was using for Var names.

One thing I noticed is this comment:

/*
* Add names of relation handled by the foreign scan when the scan is a
- * join
+ * join. The input looks something like "(1) LEFT JOIN (2)", and we must
+ * replace the digit strings with the correct relation names. We do that
+ * here, not when the plan is created, because we can't know what aliases
+ * ruleutils.c will assign at plan creation time.
*/

I think "names of relation" should be "names of relations", so how
about fixing that as well? Other than that the patch looks good to
me. Thanks for working on this! (Actually, we discussed this before.
See [1]. I wasn't able to come up with a solution, though.)

> 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.

Best regards,
Etsuro Fujita

[1] https://www.postgresql.org/message-id/c2c7191b-5ca0-b37a-9e9d-4df15ffb554b%40lab.ntt.co.jp