From 356529ff0860c9e7af91081d0dfe38c0fda04e76 Mon Sep 17 00:00:00 2001 From: James Coleman Date: Tue, 24 Mar 2020 22:06:39 -0400 Subject: [PATCH v41 3/5] fix rescan --- src/backend/commands/explain.c | 8 +-- src/backend/executor/nodeIncrementalSort.c | 55 +++++++++---------- src/include/nodes/execnodes.h | 1 - .../regress/expected/incremental_sort.out | 31 +++++++++++ src/test/regress/sql/incremental_sort.sql | 11 ++++ 5 files changed, 72 insertions(+), 34 deletions(-) diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c index cf8cfd31f5..56f8e1fd21 100644 --- a/src/backend/commands/explain.c +++ b/src/backend/commands/explain.c @@ -2817,12 +2817,12 @@ show_incremental_sort_info(IncrementalSortState *incrsortstate, IncrementalSortGroupInfo *fullsortGroupInfo; IncrementalSortGroupInfo *prefixsortGroupInfo; - if (!(es->analyze && incrsortstate->sort_Done)) + fullsortGroupInfo = &incrsortstate->incsort_info.fullsortGroupInfo; + + if (!(es->analyze && fullsortGroupInfo->groupCount > 0)) return; - fullsortGroupInfo = &incrsortstate->incsort_info.fullsortGroupInfo; - if (fullsortGroupInfo->groupCount > 0) - show_incremental_sort_group_info(fullsortGroupInfo, "Full-sort", es); + show_incremental_sort_group_info(fullsortGroupInfo, "Full-sort", es); prefixsortGroupInfo = &incrsortstate->incsort_info.prefixsortGroupInfo; if (prefixsortGroupInfo->groupCount > 0) show_incremental_sort_group_info(prefixsortGroupInfo, "Presorted", es); diff --git a/src/backend/executor/nodeIncrementalSort.c b/src/backend/executor/nodeIncrementalSort.c index 296a0c0675..53dccf3450 100644 --- a/src/backend/executor/nodeIncrementalSort.c +++ b/src/backend/executor/nodeIncrementalSort.c @@ -963,12 +963,6 @@ ExecIncrementalSort(PlanState *pstate) /* Restore to user specified direction. */ estate->es_direction = dir; - /* - * Remember that we've begun our scan and sort so we know how to handle - * rescan. - */ - node->sort_Done = true; - /* * Get the first or next tuple from tuplesort. Returns NULL if no more * tuples. @@ -1000,8 +994,7 @@ ExecInitIncrementalSort(IncrementalSort *node, EState *estate, int eflags) * EXEC_FLAG_BACKWARD or EXEC_FLAG_MARK, because we only one of many sort * batches in the current sort state. */ - Assert((eflags & (EXEC_FLAG_REWIND | - EXEC_FLAG_BACKWARD | + Assert((eflags & (EXEC_FLAG_BACKWARD | EXEC_FLAG_MARK)) == 0); /* Initialize state structure. */ @@ -1010,15 +1003,15 @@ ExecInitIncrementalSort(IncrementalSort *node, EState *estate, int eflags) incrsortstate->ss.ps.state = estate; incrsortstate->ss.ps.ExecProcNode = ExecIncrementalSort; + incrsortstate->execution_status = INCSORT_LOADFULLSORT; incrsortstate->bounded = false; - incrsortstate->sort_Done = false; incrsortstate->outerNodeDone = false; + incrsortstate->bound_Done = 0; incrsortstate->fullsort_state = NULL; incrsortstate->prefixsort_state = NULL; incrsortstate->group_pivot = NULL; incrsortstate->transfer_tuple = NULL; incrsortstate->n_fullsort_remaining = 0; - incrsortstate->bound_Done = 0; incrsortstate->presorted_keys = NULL; if (incrsortstate->ss.ps.instrument != NULL) @@ -1132,30 +1125,35 @@ ExecReScanIncrementalSort(IncrementalSortState *node) PlanState *outerPlan = outerPlanState(node); /* - * XXX: This is suspect. + * Incremental sort doesn't support efficient rescan even when paramters + * haven't changed (e.g., rewind) because unlike regular sort we don't store + * all tuples at once for the full sort. + * + * So even if EXEC_FLAG_REWIND is set we just reset all of our state and + * reexecute the sort along with the child node below us. * - * If we haven't sorted yet, just return. If outerplan's chgParam is not - * NULL then it will be re-scanned by ExecProcNode, else no reason to - * re-scan it at all. + * In theory if we've only fill the full sort with one batch (and haven't + * reset it for a new batch yet) then we could efficiently rewind, but that + * seems a narrow enough case that it's not worth handling specially at + * this time. */ - if (!node->sort_Done) - return; /* must drop pointer to sort result tuple */ ExecClearTuple(node->ss.ps.ps_ResultTupleSlot); + if (node->group_pivot != NULL) + ExecClearTuple(node->group_pivot); + if (node->transfer_tuple != NULL) + ExecClearTuple(node->transfer_tuple); + + node->bounded = false; node->outerNodeDone = false; + node->n_fullsort_remaining = 0; + node->bound_Done = 0; + node->presorted_keys = NULL; + + node->execution_status = INCSORT_LOADFULLSORT; - /* - * XXX: This is suspect. - * - * If subnode is to be rescanned then we forget previous sort results; we - * have to re-read the subplan and re-sort. Also must re-sort if the - * bounded-sort parameters changed or we didn't select randomAccess. - * - * Otherwise we can just rewind and rescan the sorted output. - */ - node->sort_Done = false; if (node->fullsort_state != NULL) { tuplesort_end(node->fullsort_state); @@ -1166,11 +1164,10 @@ ExecReScanIncrementalSort(IncrementalSortState *node) tuplesort_end(node->prefixsort_state); node->prefixsort_state = NULL; } - node->bound_Done = 0; /* - * if chgParam of subnode is not null then plan will be re-scanned by - * first ExecProcNode. + * If chgParam of subnode is not null, theni the plan will be re-scanned by + * the first ExecProcNode. */ if (outerPlan->chgParam == NULL) ExecReScan(outerPlan); diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h index 71ac1417ab..6127ab5912 100644 --- a/src/include/nodes/execnodes.h +++ b/src/include/nodes/execnodes.h @@ -2070,7 +2070,6 @@ typedef struct IncrementalSortState ScanState ss; /* its first field is NodeTag */ bool bounded; /* is the result set bounded? */ int64 bound; /* if bounded, how many tuples are needed */ - bool sort_Done; /* sort completed yet? */ bool outerNodeDone; /* finished fetching tuples from outer node */ int64 bound_Done; /* value of bound we did the sort with */ IncrementalSortExecutionStatus execution_status; diff --git a/src/test/regress/expected/incremental_sort.out b/src/test/regress/expected/incremental_sort.out index 4f6f2288a3..689143456e 100644 --- a/src/test/regress/expected/incremental_sort.out +++ b/src/test/regress/expected/incremental_sort.out @@ -602,6 +602,37 @@ select * from (select * from t order by a) s order by a, b limit 70; 9 | 70 (70 rows) +-- Test rescan. +begin; +-- We force the planner to choose a plan with incremental sort on the right side +-- of a nested loop join node. That way we trigger the rescan code path. +set local enable_hashjoin = off; +set local enable_mergejoin = off; +set local enable_material = off; +set local enable_sort = off; +explain (costs off) select * from t left join (select * from (select * from t order by a) v order by a, b) s on s.a = t.a where t.a in (1, 2); + QUERY PLAN +------------------------------------------------ + Nested Loop Left Join + Join Filter: (t_1.a = t.a) + -> Seq Scan on t + Filter: (a = ANY ('{1,2}'::integer[])) + -> Incremental Sort + Sort Key: t_1.a, t_1.b + Presorted Key: t_1.a + -> Sort + Sort Key: t_1.a + -> Seq Scan on t t_1 +(10 rows) + +select * from t left join (select * from (select * from t order by a) v order by a, b) s on s.a = t.a where t.a in (1, 2); + a | b | a | b +---+---+---+--- + 1 | 1 | 1 | 1 + 2 | 2 | 2 | 2 +(2 rows) + +rollback; -- Test EXPLAIN ANALYZE (text output) with both fullsort and presorted groups. explain (analyze, costs off, summary off, timing off) select * from (select * from t order by a) s order by a, b limit 70; diff --git a/src/test/regress/sql/incremental_sort.sql b/src/test/regress/sql/incremental_sort.sql index 9320a10b91..e567a9a14d 100644 --- a/src/test/regress/sql/incremental_sort.sql +++ b/src/test/regress/sql/incremental_sort.sql @@ -50,6 +50,17 @@ delete from t; insert into t(a, b) select (case when i < 5 then i else 9 end), i from generate_series(1, 100) n(i); 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; +-- Test rescan. +begin; +-- We force the planner to choose a plan with incremental sort on the right side +-- of a nested loop join node. That way we trigger the rescan code path. +set local enable_hashjoin = off; +set local enable_mergejoin = off; +set local enable_material = off; +set local enable_sort = off; +explain (costs off) select * from t left join (select * from (select * from t order by a) v order by a, b) s on s.a = t.a where t.a in (1, 2); +select * from t left join (select * from (select * from t order by a) v order by a, b) s on s.a = t.a where t.a in (1, 2); +rollback; -- Test EXPLAIN ANALYZE (text output) with both fullsort and presorted groups. explain (analyze, costs off, summary off, timing off) select * from (select * from t order by a) s order by a, b limit 70; -- 2.17.1