On Mon, Nov 23, 2020 at 8:19 AM James Coleman <jtc...@gmail.com> wrote: > > On Sun, Nov 22, 2020 at 5:07 PM Tomas Vondra > <tomas.von...@enterprisedb.com> wrote: > > > > On 11/22/20 10:31 PM, Tom Lane wrote: > > > Tomas Vondra <tomas.von...@enterprisedb.com> writes: > > >> On 11/20/20 11:24 PM, James Coleman wrote: > > >>> While looking at another issue I noticed that create_gather_merge_plan > > >>> calls make_sort if the subplan isn't sufficiently sorted. In all of > > >>> the cases I've seen where a gather merge path (not plan) is created > > >>> the input path is expected to be properly sorted, so I was wondering > > >>> if anyone happened to know what case is being handled by the make_sort > > >>> call. Removing it doesn't seem to break any tests. > > > > > >> Yeah, I think you're right this is dead code, essentially. We're only > > >> ever calling create_gather_merge_path() with pathkeys matching the > > >> subpath. And it's like that on REL_12_STABLE too, i.e. before the > > >> incremental sort was introduced. > > > > > > It's probably there by analogy to the other callers of > > > prepare_sort_from_pathkeys, which all do at least a conditional > > > make_sort(). I'd be inclined to leave it there; it's cheap insurance > > > against somebody weakening the existing behavior. > > > > > > > But how do we know it's safe to actually do the sort there, e.g. in > > light of the volatility/parallel-safety issues discussed in other threads? > > > > I agree the check may be useful, but maybe we should just do elog(ERROR) > > instead. > > That was my thought. I'm not a big fan of maintaining a "this might be > useful" path particularly when there isn't any case in the code or > tests at all that exercises it. In other words, not only is it not > useful [yet], but also we don't actually have any signal to know that > it works (or keeps working) -- whether through tests or production > use. > > So I'm +1 on turning it into an ERROR log instead, since it seems to > me that encountering this case would almost certainly represent a bug > in path generation.
Here's a patch to do that. James
From 798d520a30a44bc00f4f7bdb4a39e443212f0424 Mon Sep 17 00:00:00 2001 From: jcoleman <jtc...@gmail.com> Date: Sun, 29 Nov 2020 09:38:59 -0500 Subject: [PATCH v1] Error if gather merge paths aren't sufficiently sorted --- src/backend/optimizer/plan/createplan.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c index 40abe6f9f6..dd58bc5688 100644 --- a/src/backend/optimizer/plan/createplan.c +++ b/src/backend/optimizer/plan/createplan.c @@ -1793,13 +1793,14 @@ create_gather_merge_plan(PlannerInfo *root, GatherMergePath *best_path) &gm_plan->nullsFirst); - /* Now, insert a Sort node if subplan isn't sufficiently ordered */ + /* + * All gather merge paths should have already guaranteed the necessary sort + * order either by adding an explicit sort node or by using presorted input. + * If not we have no guarantee that applying a sort at this point would be + * parallel safe, + */ if (!pathkeys_contained_in(pathkeys, best_path->subpath->pathkeys)) - subplan = (Plan *) make_sort(subplan, gm_plan->numCols, - gm_plan->sortColIdx, - gm_plan->sortOperators, - gm_plan->collations, - gm_plan->nullsFirst); + elog(ERROR, "gather merge input not sufficiently sorted"); /* Now insert the subplan under GatherMerge. */ gm_plan->plan.lefttree = subplan; -- 2.17.1
