From 541a7f0ae6060763cd4448359159c1a2c5980a68 Mon Sep 17 00:00:00 2001 From: Peter Geoghegan Date: Thu, 13 Oct 2016 10:54:31 -0700 Subject: [PATCH 2/3] Avoid copying within tuplesort_gettupleslot() Add a "copy" argument to make it optional to receive a copy of caller tuple that is safe to use across tuplesort_gettupleslot() calls, as a performance optimization. Existing tuplesort_gettupleslot() callers are made to opt out of copying where that has been determined to be safe. This brings tuplesort_gettupleslot() in line with tuplestore_gettupleslot(). In the future, a "copy" tuplesort_getdatum() argument may be added, that similarly allows callers to opt out of receiving a new copy of tuple under their direct ownership. --- src/backend/executor/nodeAgg.c | 9 ++++++--- src/backend/executor/nodeSort.c | 5 +++-- src/backend/utils/adt/orderedsetaggs.c | 5 +++-- src/backend/utils/sort/tuplesort.c | 17 +++++++++++------ src/include/utils/tuplesort.h | 2 +- 5 files changed, 24 insertions(+), 14 deletions(-) diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c index eefb3d6..16a1263 100644 --- a/src/backend/executor/nodeAgg.c +++ b/src/backend/executor/nodeAgg.c @@ -570,6 +570,9 @@ initialize_phase(AggState *aggstate, int newphase) * Fetch a tuple from either the outer plan (for phase 0) or from the sorter * populated by the previous phase. Copy it to the sorter for the next phase * if any. + * + * Callers cannot rely on memory for tuple in returned slot remaining allocated + * past any subsequent call here. (The sorter may recycle the memory.) */ static TupleTableSlot * fetch_input_tuple(AggState *aggstate) @@ -578,8 +581,8 @@ fetch_input_tuple(AggState *aggstate) if (aggstate->sort_in) { - if (!tuplesort_gettupleslot(aggstate->sort_in, true, aggstate->sort_slot, - NULL)) + if (!tuplesort_gettupleslot(aggstate->sort_in, true, false, + aggstate->sort_slot, NULL)) return NULL; slot = aggstate->sort_slot; } @@ -1273,7 +1276,7 @@ process_ordered_aggregate_multi(AggState *aggstate, ExecClearTuple(slot2); while (tuplesort_gettupleslot(pertrans->sortstates[aggstate->current_set], - true, slot1, &newAbbrevVal)) + true, true, slot1, &newAbbrevVal)) { /* * Extract the first numTransInputs columns as datums to pass to the diff --git a/src/backend/executor/nodeSort.c b/src/backend/executor/nodeSort.c index a34dcc5..8fc7106 100644 --- a/src/backend/executor/nodeSort.c +++ b/src/backend/executor/nodeSort.c @@ -132,12 +132,13 @@ ExecSort(SortState *node) /* * Get the first or next tuple from tuplesort. Returns NULL if no more - * tuples. + * tuples. Note that we rely on memory for tuple in slot remaining + * allocated only until subsequent call here. */ slot = node->ss.ps.ps_ResultTupleSlot; (void) tuplesort_gettupleslot(tuplesortstate, ScanDirectionIsForward(dir), - slot, NULL); + false, slot, NULL); return slot; } diff --git a/src/backend/utils/adt/orderedsetaggs.c b/src/backend/utils/adt/orderedsetaggs.c index fe44d56..3c23329 100644 --- a/src/backend/utils/adt/orderedsetaggs.c +++ b/src/backend/utils/adt/orderedsetaggs.c @@ -1189,7 +1189,7 @@ hypothetical_rank_common(FunctionCallInfo fcinfo, int flag, tuplesort_performsort(osastate->sortstate); /* iterate till we find the hypothetical row */ - while (tuplesort_gettupleslot(osastate->sortstate, true, slot, NULL)) + while (tuplesort_gettupleslot(osastate->sortstate, true, true, slot, NULL)) { bool isnull; Datum d = slot_getattr(slot, nargs + 1, &isnull); @@ -1352,7 +1352,8 @@ hypothetical_dense_rank_final(PG_FUNCTION_ARGS) slot2 = extraslot; /* iterate till we find the hypothetical row */ - while (tuplesort_gettupleslot(osastate->sortstate, true, slot, &abbrevVal)) + while (tuplesort_gettupleslot(osastate->sortstate, true, true, slot, + &abbrevVal)) { bool isnull; Datum d = slot_getattr(slot, nargs + 1, &isnull); diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c index 71524c2..d60159a 100644 --- a/src/backend/utils/sort/tuplesort.c +++ b/src/backend/utils/sort/tuplesort.c @@ -2083,12 +2083,15 @@ tuplesort_gettuple_common(Tuplesortstate *state, bool forward, * NULL value in leading attribute will set abbreviated value to zeroed * representation, which caller may rely on in abbreviated inequality check. * - * The slot receives a copied tuple (sometimes allocated in caller memory - * context) that will stay valid regardless of future manipulations of the - * tuplesort's state. + * If copy is TRUE, the slot receives a copied tuple that will stay valid + * regardless of future manipulations of the tuplesort's state. Memory is + * owned by the caller. If copy is FALSE, the slot may just receive a pointer + * to a tuple held within the tuplesort. The latter is more efficient, but + * the slot contents may be corrupted if there is another call here before + * previous slot contents are used. */ bool -tuplesort_gettupleslot(Tuplesortstate *state, bool forward, +tuplesort_gettupleslot(Tuplesortstate *state, bool forward, bool copy, TupleTableSlot *slot, Datum *abbrev) { MemoryContext oldcontext = MemoryContextSwitchTo(state->sortcontext); @@ -2105,8 +2108,10 @@ tuplesort_gettupleslot(Tuplesortstate *state, bool forward, if (state->sortKeys->abbrev_converter && abbrev) *abbrev = stup.datum1; - stup.tuple = heap_copy_minimal_tuple((MinimalTuple) stup.tuple); - ExecStoreMinimalTuple((MinimalTuple) stup.tuple, slot, true); + if (copy) + stup.tuple = heap_copy_minimal_tuple((MinimalTuple) stup.tuple); + + ExecStoreMinimalTuple((MinimalTuple) stup.tuple, slot, copy); return true; } else diff --git a/src/include/utils/tuplesort.h b/src/include/utils/tuplesort.h index 38af746..ad7adca 100644 --- a/src/include/utils/tuplesort.h +++ b/src/include/utils/tuplesort.h @@ -93,7 +93,7 @@ extern void tuplesort_putdatum(Tuplesortstate *state, Datum val, extern void tuplesort_performsort(Tuplesortstate *state); extern bool tuplesort_gettupleslot(Tuplesortstate *state, bool forward, - TupleTableSlot *slot, Datum *abbrev); + bool copy, TupleTableSlot *slot, Datum *abbrev); extern HeapTuple tuplesort_getheaptuple(Tuplesortstate *state, bool forward); extern IndexTuple tuplesort_getindextuple(Tuplesortstate *state, bool forward); extern bool tuplesort_getdatum(Tuplesortstate *state, bool forward, -- 2.7.4