Re: BUG #18902: TRAP:: failed Assert("!is_sorted") in File: "createplan.c" - Mailing list pgsql-bugs

On Thu, Apr 24, 2025 at 6:20 PM Richard Guo <guofenglinux@gmail.com> wrote:
> On Thu, Apr 24, 2025 at 5:40 PM Richard Guo <guofenglinux@gmail.com> wrote:
> > Yeah.  Quoting the comments for outersortkeys/innersortkeys:
> >
> >  * outersortkeys (resp. innersortkeys) is NIL if the outer path
> >  * (resp. inner path) is already ordered appropriately for the
> >  * mergejoin.  If it is not NIL then it is a PathKeys list describing
> >  * the ordering that must be created by an explicit Sort node.
> >
> > And try_mergejoin_path will detect whether the outer path (resp. inner
> > path) is already well enough ordered, and suppresses an explicit sort
> > step if so by setting outersortkeys (resp. innersortkeys) to NIL.
> >
> > This reflects a basic assumption: if MergePath.outersortkeys is not
> > NIL, it means the outer path is not sufficiently ordered.
> >
> > Therefore, I think the "Assert(!is_sorted)" when outersortkeys is not
> > NIL is reasonable.  And I think some other code is violating this
> > assumption.
> >
> > I tried this repro query on v17 and got a plan with a redundant Sort
> > node.  It seems that this issue existed prior to commit 828e94c9d, and
> > the Assert introduced in that commit makes the issue easier to detect.
>
> It seems the culprit is in GetExistingLocalJoinPath(), which is used
> to get a local path for EPQ checks.  In that function, if the outer or
> inner path of the selected join path is a ForeignScan, it is replaced
> with the fdw_outerpath.
>
> The problem arises when the selected join path is a MergePath, and its
> outer or inner path is a ForeignScan that is not already well enough
> ordered.  In this case, the MergePath will have non-NIL outersortkeys
> or innersortkeys.  If we then replace the outer or inner path with the
> corresponding fdw_outerpath, and that path is already sufficiently
> ordered, we end up in an inconsistent state: the MergePath has non-NIL
> outersortkeys/innersortkeys, and its input path is already properly
> ordered.
>
> This inconsistency leads to a redundant Sort node prior to commit
> 828e94c9d, and triggers an Assert failure after that commit.

To fix, I think we can set outersortkeys/innersortkeys to NIL in
GetExistingLocalJoinPath() if we detect that the new outer/inner path
of the MergePath is already sorted properly, similar to what we do in
try_mergejoin_path().

    if (IS_JOIN_REL(foreign_path->path.parent))
+   {
        joinpath->outerjoinpath = foreign_path->fdw_outerpath;
+
+       if (joinpath->path.pathtype == T_MergeJoin)
+       {
+           MergePath   *merge_path = (MergePath *) joinpath;
+
+           /*
+            * If the new outer path is already well enough ordered for
+            * mergejoin, we can skip doing an explicit sort.
+            */
+           if (merge_path->outersortkeys &&
+               pathkeys_contained_in(merge_path->outersortkeys,
+                                     joinpath->outerjoinpath->pathkeys))
+               merge_path->outersortkeys = NIL;
+       }
+   }

(We need to do the same to innersortkeys.)

One problem with this approach is that the cost of the explicit sort
has already been included in the MergePath.  However, I'm not too
concerned about this, since the resulting plan is only used for EPQ
checks, where cost accuracy is not that important.  After all, we also
don't adjust the join path's cost when replacing its outer or inner
subpath.

Any thoughts?

Thanks
Richard