On Fri, 25 Apr 2025 at 14:16, Richard Guo <guofenglinux@gmail.com> wrote:
> Hmm, well, I don't think we can get rid of MergePath.outersortkeys and
> innersortkeys, even if we've cached the number of presorted keys for
> the outer and inner paths. These fields represent the desired
> ordering to be produced by an explicit Sort node — not the current
> ordering of the outer or inner path. Without this information, how
> would create_mergejoin_plan know what sort order to generate for the
> outer and inner paths if they are not already sorted?
I misspoke about getting rid of the two List fields.
> Maybe we could cache the number of presorted keys for the outer and
> inner paths in MergePath to save some pathkeys_count_contained_in
> calls. But I'm not too sure it's worthwhile. The pathkeys are
> canonical, and can be checked for equality by simple pointer
> comparison. So it does not seem to cost too much. Besides, the
> "redundant" pathkey checks actually helped uncover the bug we're
> discussing here — didn't they?
I don't see any reason why we couldn't keep an Assert to ensure the
sorted-ness of the Path matches our expectations. Calculating the
whole thing again in non-assert builds seems a bit silly. The
previous code took care to avoid recalculations by nullifying the
Lists when no sort was required, you've not followed that with the
incremental sort changes, and to me, that makes it feel a little
half-done.
David
