On Sun, Apr 27, 2025 at 9:14 AM David Rowley <dgrowleyml@gmail.com> wrote: > On Fri, 25 Apr 2025 at 14:16, Richard Guo <guofenglinux@gmail.com> wrote: > > 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.
Fair point. Here is the patchset doing that. 0001 fixes this bug by setting outersortkeys/innersortkeys to NIL in GetExistingLocalJoinPath if we detect that the new outer/inner path of the MergePath is already sorted properly.
The 0001 fixing this bug looks good to me.
0002 caches the number of presorted keys of the outer path in MergePath, allowing us to save several calls to pathkeys_count_contained_in.
I quickly look through the 0002, it seems good to me.
(I'm a bit hesitant about whether we should apply the same caching to the inner path of a mergejoin. I chose not to do that in this patch, since incremental sort is not used for the inner path of a mergejoin.)
