Re: Bogus EXPLAIN results with column aliases for mismatched partitions

On Sun, Dec 1, 2019 at 11:40 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> then the EXPLAIN output produced by HEAD looks like:
>
>                   QUERY PLAN
> -----------------------------------------------
>  Sort
>    Output: p.x, p.b
>    Sort Key: p.x
>    ->  Append
>          ->  Seq Scan on public.part_p1 p
>                Output: p.x, p.b
>          ->  Seq Scan on public.part_rev p_1
>                Output: p_1.a, p_1.x
>          ->  Seq Scan on public.part_p2_p1 p_2
>                Output: p_2.x, p_2.b
> (10 rows)
>
> wherein the "x" alias for column "a" has been applied to part_rev.b.
> That's wrong and confusing.

Agreed.

> The reason this happens is that expand_single_inheritance_child()
> just clones the parent RTE's alias node without any thought for
> the possibility that the columns don't match one-to-one.  It's
> an ancient bug that affects traditional inheritance as well as
> partitioning.
>
> I experimented with fixing this by making expand_single_inheritance_child
> generate a correctly-adjusted child alias node, which seems reasonable
> since it takes pains to adjust the rest of the child RTE for the different
> column layout.  It turns out to be slightly tedious to do that without
> causing a lot of regression test diffs: if we add an alias node where
> there was none before, that affects ruleutils.c's selection of table
> aliases not just column aliases.  The "variant-a" patch below mostly
> succeeds in avoiding test diffs, but it adds a fair amount of complication
> to inherit.c.  The "variant-b" patch below uses a simpler way of setting
> up the child aliases, which results in a whole lot of test diffs: all
> children of a parent named "x" will now show in EXPLAIN with aliases like
> "x_1", "x_2", etc.  (That happens already if you wrote an explicit table
> alias "x", but not if you didn't.)  While my initial reaction was that
> that was an unacceptable amount of churn, the idea gets more appealing the
> more I think about it.  It means that tables you did not name in the query
> will be shown with aliases that clearly identify their connection to
> something you did name.  So despite the added churn, I'm kind of attracted
> to the variant-b approach.  (Note that the discussion in [1] is almost
> certainly going to result in some changes to ruleutils.c's alias-selection
> behavior anyway, so I don't think staying exactly compatible with v12 is
> worth much here.)
>
> On the other hand, if we went with variant-a it might be plausible to
> back-patch this fix.  But given the fact that this is a mostly cosmetic
> problem, and we've not had field complaints, I don't feel a big need
> to fix it in the back branches.

I tend to agree that "variant b" is more appealing for the reason that
it makes the connection between child RTEs and that of the table named
in the query from which they originate more explicit.

> * To make computing the modified column alias list cheap, I made
> make_inh_translation_list() compute a reverse-mapping array showing the
> parent column associated with each child column.  I'm more than a little
> bit tempted to add that array to the AppendRelInfo struct, instead of
> passing it back separately and then discarding it.  We could use the info
> to simplify and speed up the reverse-mapping logic added by commit
> 553d2ec27, and I bet there will be other use-cases later.  But I've not
> done that in these patches.

+1

Thanks,
Amit