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

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