Re: BUG #18902: TRAP:: failed Assert("!is_sorted") in File: "createplan.c"

On Sun, Apr 27, 2025 at 9:14 AM David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
> On Fri, 25 Apr 2025 at 14:16, Richard Guo <guofenglinux(at)gmail(dot)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. 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'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.)

Thanks
Richard