Allow avoiding tuple copy 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.

Author: Peter Geoghegan
Discussion: CAM3SWZQWZZ_N=DmmL7tKy_OUjGH_5mN=N=A6h7kHyyDvEhg2DA@mail.gmail.com

diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index ef35da6ade6ac051dd448d9954b0724730a7d490..0109aee1fd8b864903d3be2de802a557738a348d 100644 (file)
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -666,6 +666,9 @@ initialize_phase(AggState *aggstate, int newphase)
  * Fetch a tuple from either the outer plan (for phase 1) 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 subsequently fetched tuple.
  */
 static TupleTableSlot *
 fetch_input_tuple(AggState *aggstate)
@@ -674,8 +677,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;
        }
@@ -1409,7 +1412,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 591a31aa6aba14027be83cd6846c44c2a016a40e..924b458df87f602904cb993824c1c8f0773de978 100644 (file)
--- 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 only rely on slot tuple remaining valid until the
+        * next fetch from the 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 e462fbd539d547bc3e13f4f9c355ef598480042b..8502fcfc82e6dbb4c127dd926091b7e93ce4e912 100644 (file)
--- 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 65cda52714b351ccb860e47e3853e93153edb923..5f62cd5962a251c98413db3a41ec681ff5b12bf2 100644 (file)
--- a/src/backend/utils/sort/tuplesort.c
+++ b/src/backend/utils/sort/tuplesort.c
@@ -1852,7 +1852,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,
@@ -2101,12 +2102,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'll 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);
@@ -2123,8 +2127,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
@@ -2137,8 +2143,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 may not rely on tuple
+ * remaining valid after any further manipulation of tuplesort.
  */
 HeapTuple
 tuplesort_getheaptuple(Tuplesortstate *state, bool forward)
@@ -2157,8 +2163,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 may not rely on tuple
+ * 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 9719db42b94cc93836f9fd1e1bec66c0c34682ad..14b9026fb7ffcb10555bcd60b31b31ef22b5825d 100644 (file)
--- a/src/include/utils/tuplesort.h
+++ b/src/include/utils/tuplesort.h
@@ -95,7 +95,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,