On Wed, Jan 25, 2017 at 3:11 PM, Peter Geoghegan <p...@heroku.com> wrote:
> On Wed, Jan 25, 2017 at 3:11 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>> Please. You might want to hit the existing ones with a separate patch,
>> but it doesn't much matter; I'd be just as happy with a patch that did
>> both things.
>
> Got it.
Attached is a patch that does both things at once. The internal
function tuplesort_gettuple_common() still mentions repeated fetches,
since that context matters to its internal callers. The various
external-facing functions have a simpler, stricter contract, as
discussed.
I didn't end up adding a line like "copy=FALSE is recommended only
when the next tuplesort manipulation will be another
tuplesort_gettupleslot fetch into the same slot", which you suggested.
When the tuplesort state machine is in any state following
"performing" a sort, there are very few remaining sane manipulations.
Just things like skipping or seeking around for tuples, and actually
ending the tuplesort and releasing resources.
--
Peter Geoghegan
From 5351b5db257cb39832647d9117465c0217e6268b Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <p...@bowt.ie>
Date: Thu, 13 Oct 2016 10:54:31 -0700
Subject: [PATCH 1/2] 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 following a subsequent manipulating of
tuplesort's state. This is a performance optimization. Most existing
tuplesort_gettupleslot() callers are made to opt out of copying.
Existing callers that happen to rely on the validity of tuple memory
beyond subsequent manipulations of the tuplesort request their own copy.
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 their own copy of tuple.
In passing, clarify assumptions that callers of other tuplesort fetch
routines may make about tuple memory validity, per gripe from Tom Lane.
---
src/backend/executor/nodeAgg.c | 10 +++++++---
src/backend/executor/nodeSort.c | 5 +++--
src/backend/utils/adt/orderedsetaggs.c | 5 +++--
src/backend/utils/sort/tuplesort.c | 28 +++++++++++++++++-----------
src/include/utils/tuplesort.h | 2 +-
5 files changed, 31 insertions(+), 19 deletions(-)
diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index aa08152..b650f71 100644
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -570,6 +570,10 @@ 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 valid
+ * past any subsequent manipulation of the sorter, such as another fetch of
+ * input from sorter. (The sorter may recycle memory.)
*/
static TupleTableSlot *
fetch_input_tuple(AggState *aggstate)
@@ -578,8 +582,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;
}
@@ -1272,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 591a31a..d901034 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 valid
+ * only until some subsequent manipulation of tuplesort.
*/
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 e462fbd..8502fcf 100644
--- a/src/backend/utils/adt/orderedsetaggs.c
+++ b/src/backend/utils/adt/orderedsetaggs.c
@@ -1190,7 +1190,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);
@@ -1353,7 +1353,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 e1e692d..af79ab3 100644
--- a/src/backend/utils/sort/tuplesort.c
+++ b/src/backend/utils/sort/tuplesort.c
@@ -1842,7 +1842,8 @@ tuplesort_performsort(Tuplesortstate *state)
* Internal routine to fetch the next tuple in either forward or back
* direction into *stup. Returns FALSE if no more tuples.
* Returned tuple belongs to tuplesort memory context, and must not be freed
- * by caller. Caller should not use tuple following next call here.
+ * by caller. Note that fetched tuple is stored in memory that may be
+ * recycled by any future fetch.
*/
static bool
tuplesort_gettuple_common(Tuplesortstate *state, bool forward,
@@ -2091,12 +2092,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 will just receive a
+ * pointer to a tuple held within the tuplesort, which is more efficient,
+ * but only safe for callers that are prepared to have any subsequent
+ * manipulation of the tuplesort's state invalidate slot contents.
*/
bool
-tuplesort_gettupleslot(Tuplesortstate *state, bool forward,
+tuplesort_gettupleslot(Tuplesortstate *state, bool forward, bool copy,
TupleTableSlot *slot, Datum *abbrev)
{
MemoryContext oldcontext = MemoryContextSwitchTo(state->sortcontext);
@@ -2113,8 +2117,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
@@ -2127,8 +2133,8 @@ tuplesort_gettupleslot(Tuplesortstate *state, bool forward,
/*
* Fetch the next tuple in either forward or back direction.
* Returns NULL if no more tuples. Returned tuple belongs to tuplesort memory
- * context, and must not be freed by caller. Caller should not use tuple
- * following next call here.
+ * context, and must not be freed by caller. Caller should not rely on tuple
+ * memory remaining valid after any further manipulation of tuplesort.
*/
HeapTuple
tuplesort_getheaptuple(Tuplesortstate *state, bool forward)
@@ -2147,8 +2153,8 @@ tuplesort_getheaptuple(Tuplesortstate *state, bool forward)
/*
* Fetch the next index tuple in either forward or back direction.
* Returns NULL if no more tuples. Returned tuple belongs to tuplesort memory
- * context, and must not be freed by caller. Caller should not use tuple
- * following next call here.
+ * context, and must not be freed by caller. Caller should not rely on tuple
+ * memory remaining valid after any further manipulation of tuplesort.
*/
IndexTuple
tuplesort_getindextuple(Tuplesortstate *state, bool forward)
diff --git a/src/include/utils/tuplesort.h b/src/include/utils/tuplesort.h
index 5b3f475..32b287c 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
