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

Thanks for taking a look and sorry about the delay in replying.

On Fri, Oct 25, 2019 at 1:51 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 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.

Actually, there is adjust_child_relids_multilevel() which translates
the top parent relids in em_nullable_relids to child relids.

I have updated the patches that way.

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

Hmm, I agree we should try to fix that situation somehow.  Even better
if we could do away with child expressions in ECs, because they cause
EC related code to show up in profiles when executing complex queries
with thousands of partitions.

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

I was wrong about that.  Fixed.

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

Actually, as of 3373c71553, add_child_rel_equivalences() looks at
parent_rel->eclass_indexes to look up ECs, so maybe we can't take out
parent_rel.

>  At the very least,
> if we keep the extra arguments, we should document them as being just
> optimizations.

For common cases that doesn't involve multi-level partitioning, it
really helps to have the appinfos be supplied by the caller because
they're already available.  I've added a comment at the top about
that.

Attached updated patches.

Thanks,
Amit