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

Richard Guo <guofenglinux@gmail.com> 于2025年5月2日周五 15:03写道：

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

-- 
Thanks,
Tender Wang