Re: v12.0: ERROR: could not find pathkey item to sort

Amit Langote <amitlangote09@gmail.com> writes:
> On Mon, Oct 14, 2019 at 11:54 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> In view of the proposed patches being dependent on some other
>> 13-only changes, I wonder if we should fix v12 by reverting
>> d25ea0127.  The potential planner performance loss for large
>> partition sets could be nasty, but failing to plan at all is worse.

> Actually, the patch I proposed to fix equivalence code can be applied
> on its own.  The example I gave on that thread needs us to fix
> partitionwise code to even work, which is perhaps a 13-only change,
> but we have an example here that is broken due to d25ea01275.
> Perhaps, considering applying my patch seems better than alternatives
> which are either reverting d25ea01275 or avoiding doing partitionwise
> join for such queries.

I looked at this a bit, and I see that the core idea is to generate
the missing EC members by applying add_child_rel_equivalences when
building child join rels.  Perhaps we can make that work, but this
patch fails to, because you've ignored the comment pointing out
that the nullable_relids fixup logic only works for baserels:

                 * And likewise for nullable_relids.  Note this code assumes
                 * parent and child relids are singletons.

We could improve that to work for joinrels, I think, but it would become
very much more complicated (and slower).  AFAICS the logic would have
to be "iterate through top_parent_relids, see which ones are in
em_nullable_relids, and for each one that is, find the corresponding
child_relid and substitute that in new_nullable_relids".  This is a
bit of a problem because we don't have any really convenient way to
map individual top parent relids to child relids.  I wonder if we
should extend AppendRelInfo to include the top parent relid as well as
the immediate parent.  (Perhaps, while we're at it, we could make
adjust_appendrel_attrs_multilevel less of an inefficient and
underdocumented mess.)

Also, I'm pretty sure this addition is wrong/broken:

+
+                /*
+                 * There aren't going to be more expressions to translate in
+                 * the same EC.
+                 */
+                break;

What makes you think that an EC can only contain one entry per rel?

More generally, as long as this patch requires changing
add_child_rel_equivalences' API anyway, I wonder if we should
rethink that altogether.  Looking at the code now, I realize that
d25ea01275 resulted in horribly bastardizing that function's API,
because the parent_rel and appinfo arguments are only consulted in
some cases, while in other cases we disregard them and rely on
child_rel->top_parent_relids to figure out how to translate stuff.
It would be possible to make the argument list be just (root, child_rel)
and always rely on child_rel->top_parent_relids.  At the very least,
if we keep the extra arguments, we should document them as being just
optimizations.

			regards, tom lane