From 746c533c1f56a286d21f241d1c88c48b4227bf3f Mon Sep 17 00:00:00 2001 From: jcoleman Date: Wed, 10 Feb 2021 17:35:41 -0500 Subject: [PATCH v1] Small cleanup for incremental sort bounded mode transition bug - Add further comments explaining why the test case triggers the issue. - Guard against the regression test changing plans. - Simplify bug fix itself. --- src/backend/executor/nodeIncrementalSort.c | 9 +-------- src/test/regress/expected/incremental_sort.out | 15 +++++++++++++++ src/test/regress/sql/incremental_sort.sql | 4 ++++ 3 files changed, 20 insertions(+), 8 deletions(-) diff --git a/src/backend/executor/nodeIncrementalSort.c b/src/backend/executor/nodeIncrementalSort.c index 82fa800cb1..9da00a0d21 100644 --- a/src/backend/executor/nodeIncrementalSort.c +++ b/src/backend/executor/nodeIncrementalSort.c @@ -345,8 +345,6 @@ switchToPresortedPrefixMode(PlanState *pstate) */ for (;;) { - lastTuple = node->n_fullsort_remaining - nTuples == 1; - /* * When we encounter multiple prefix key groups inside the full sort * tuplesort we have to carry over the last read tuple into the next @@ -395,17 +393,12 @@ switchToPresortedPrefixMode(PlanState *pstate) */ ExecClearTuple(node->group_pivot); - /* - * Also make sure we take the didn't-consume-all-the-tuples - * path below, even if this happened to be the last tuple of - * the batch. - */ - lastTuple = false; break; } } firstTuple = false; + lastTuple = node->n_fullsort_remaining - nTuples == 0; /* * If we've copied all of the tuples from the full sort state into the diff --git a/src/test/regress/expected/incremental_sort.out b/src/test/regress/expected/incremental_sort.out index d574583844..68ca321163 100644 --- a/src/test/regress/expected/incremental_sort.out +++ b/src/test/regress/expected/incremental_sort.out @@ -676,6 +676,21 @@ select * from (select * from t order by a) s order by a, b limit 70; (70 rows) -- Checks case where we hit a group boundary at the last tuple of a batch. +-- Because the full sort state is bounded, we scan 64 tuples (the mode +-- transition point) but only retain 5. Thus when we transition modes, all +-- tuples in the full sort state have different prefix keys. +explain (costs off) select * from (select * from t order by a) s order by a, b limit 5; + QUERY PLAN +--------------------------------- + Limit + -> Incremental Sort + Sort Key: t.a, t.b + Presorted Key: t.a + -> Sort + Sort Key: t.a + -> Seq Scan on t +(7 rows) + select * from (select * from t order by a) s order by a, b limit 5; a | b ---+--- diff --git a/src/test/regress/sql/incremental_sort.sql b/src/test/regress/sql/incremental_sort.sql index 9965fcd777..81429164d4 100644 --- a/src/test/regress/sql/incremental_sort.sql +++ b/src/test/regress/sql/incremental_sort.sql @@ -150,6 +150,10 @@ analyze t; explain (costs off) select * from (select * from t order by a) s order by a, b limit 70; select * from (select * from t order by a) s order by a, b limit 70; -- Checks case where we hit a group boundary at the last tuple of a batch. +-- Because the full sort state is bounded, we scan 64 tuples (the mode +-- transition point) but only retain 5. Thus when we transition modes, all +-- tuples in the full sort state have different prefix keys. +explain (costs off) select * from (select * from t order by a) s order by a, b limit 5; select * from (select * from t order by a) s order by a, b limit 5; -- Test rescan. -- 2.17.1