Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 0e52903

Browse filesBrowse files
tglsfdctglsfdc
committed
Simplify loop logic in nodeIncrementalSort.c.
The inner loop in switchToPresortedPrefixMode() can be implemented as a conventional integer-counter for() loop, removing a couple of redundant boolean state variables. The old logic here was a remnant of earlier development, but as things now stand there's no reason for extra complexity. Also, annotate the test case added by 82e0e29 to explain why it manages to hit the corner case fixed in that commit, and add an EXPLAIN to verify that it's creating an incremental-sort plan. Back-patch to v13, like the previous patch. James Coleman and Tom Lane Discussion: https://postgr.es/m/16846-ae49f51ac379a4cb@postgresql.org
1 parent 54e51dc commit 0e52903
Copy full SHA for 0e52903

File tree

Expand file treeCollapse file tree

3 files changed

+30
-34
lines changed
Filter options
Expand file treeCollapse file tree

3 files changed

+30
-34
lines changed

‎src/backend/executor/nodeIncrementalSort.c

Copy file name to clipboardExpand all lines: src/backend/executor/nodeIncrementalSort.c
+11-34Lines changed: 11 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -288,9 +288,7 @@ switchToPresortedPrefixMode(PlanState *pstate)
288288
{
289289
IncrementalSortState *node = castNode(IncrementalSortState, pstate);
290290
ScanDirection dir;
291-
int64 nTuples = 0;
292-
bool lastTuple = false;
293-
bool firstTuple = true;
291+
int64 nTuples;
294292
TupleDesc tupDesc;
295293
PlanState *outerNode;
296294
IncrementalSort *plannode = castNode(IncrementalSort, node->ss.ps.plan);
@@ -343,20 +341,16 @@ switchToPresortedPrefixMode(PlanState *pstate)
343341
* Copy as many tuples as we can (i.e., in the same prefix key group) from
344342
* the full sort state to the prefix sort state.
345343
*/
346-
for (;;)
344+
for (nTuples = 0; nTuples < node->n_fullsort_remaining; nTuples++)
347345
{
348-
lastTuple = node->n_fullsort_remaining - nTuples == 1;
349-
350346
/*
351347
* When we encounter multiple prefix key groups inside the full sort
352348
* tuplesort we have to carry over the last read tuple into the next
353349
* batch.
354350
*/
355-
if (firstTuple && !TupIsNull(node->transfer_tuple))
351+
if (nTuples == 0 && !TupIsNull(node->transfer_tuple))
356352
{
357353
tuplesort_puttupleslot(node->prefixsort_state, node->transfer_tuple);
358-
nTuples++;
359-
360354
/* The carried over tuple is our new group pivot tuple. */
361355
ExecCopySlot(node->group_pivot, node->transfer_tuple);
362356
}
@@ -376,7 +370,6 @@ switchToPresortedPrefixMode(PlanState *pstate)
376370
if (isCurrentGroup(node, node->group_pivot, node->transfer_tuple))
377371
{
378372
tuplesort_puttupleslot(node->prefixsort_state, node->transfer_tuple);
379-
nTuples++;
380373
}
381374
else
382375
{
@@ -395,27 +388,10 @@ switchToPresortedPrefixMode(PlanState *pstate)
395388
*/
396389
ExecClearTuple(node->group_pivot);
397390

398-
/*
399-
* Also make sure we take the didn't-consume-all-the-tuples
400-
* path below, even if this happened to be the last tuple of
401-
* the batch.
402-
*/
403-
lastTuple = false;
391+
/* Break out of for-loop early */
404392
break;
405393
}
406394
}
407-
408-
firstTuple = false;
409-
410-
/*
411-
* If we've copied all of the tuples from the full sort state into the
412-
* prefix sort state, then we don't actually know that we've yet found
413-
* the last tuple in that prefix key group until we check the next
414-
* tuple from the outer plan node, so we retain the current group
415-
* pivot tuple prefix key group comparison.
416-
*/
417-
if (lastTuple)
418-
break;
419395
}
420396

421397
/*
@@ -428,14 +404,15 @@ switchToPresortedPrefixMode(PlanState *pstate)
428404
node->n_fullsort_remaining -= nTuples;
429405
SO1_printf("Setting n_fullsort_remaining to " INT64_FORMAT "\n", node->n_fullsort_remaining);
430406

431-
if (lastTuple)
407+
if (node->n_fullsort_remaining == 0)
432408
{
433409
/*
434-
* We've confirmed that all tuples remaining in the full sort batch is
435-
* in the same prefix key group and moved all of those tuples into the
436-
* presorted prefix tuplesort. Now we can save our pivot comparison
437-
* tuple and continue fetching tuples from the outer execution node to
438-
* load into the presorted prefix tuplesort.
410+
* We've found that all tuples remaining in the full sort batch are in
411+
* the same prefix key group and moved all of those tuples into the
412+
* presorted prefix tuplesort. We don't know that we've yet found the
413+
* last tuple in the current prefix key group, so save our pivot
414+
* comparison tuple and continue fetching tuples from the outer
415+
* execution node to load into the presorted prefix tuplesort.
439416
*/
440417
ExecCopySlot(node->group_pivot, node->transfer_tuple);
441418
SO_printf("Setting execution_status to INCSORT_LOADPREFIXSORT (switchToPresortedPrefixMode)\n");

‎src/test/regress/expected/incremental_sort.out

Copy file name to clipboardExpand all lines: src/test/regress/expected/incremental_sort.out
+15Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -676,6 +676,21 @@ select * from (select * from t order by a) s order by a, b limit 70;
676676
(70 rows)
677677

678678
-- Checks case where we hit a group boundary at the last tuple of a batch.
679+
-- Because the full sort state is bounded, we scan 64 tuples (the mode
680+
-- transition point) but only retain 5. Thus when we transition modes, all
681+
-- tuples in the full sort state have different prefix keys.
682+
explain (costs off) select * from (select * from t order by a) s order by a, b limit 5;
683+
QUERY PLAN
684+
---------------------------------
685+
Limit
686+
-> Incremental Sort
687+
Sort Key: t.a, t.b
688+
Presorted Key: t.a
689+
-> Sort
690+
Sort Key: t.a
691+
-> Seq Scan on t
692+
(7 rows)
693+
679694
select * from (select * from t order by a) s order by a, b limit 5;
680695
a | b
681696
---+---

‎src/test/regress/sql/incremental_sort.sql

Copy file name to clipboardExpand all lines: src/test/regress/sql/incremental_sort.sql
+4Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,10 @@ analyze t;
150150
explain (costs off) select * from (select * from t order by a) s order by a, b limit 70;
151151
select * from (select * from t order by a) s order by a, b limit 70;
152152
-- Checks case where we hit a group boundary at the last tuple of a batch.
153+
-- Because the full sort state is bounded, we scan 64 tuples (the mode
154+
-- transition point) but only retain 5. Thus when we transition modes, all
155+
-- tuples in the full sort state have different prefix keys.
156+
explain (costs off) select * from (select * from t order by a) s order by a, b limit 5;
153157
select * from (select * from t order by a) s order by a, b limit 5;
154158

155159
-- Test rescan.

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.