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

Amit Langote <amitlangote09(at)gmail(dot)com> writes:
> On Mon, Oct 14, 2019 at 11:54 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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