Allow nodeSort to perform Datum sorts for byref types

Here we add a new 'copy' parameter to tuplesort_getdatum so that we can
instruct the function not to datumCopy() byref Datums before returning.

Similar to 91e9e89dc, this can provide significant performance
improvements in nodeSort when sorting by a single byref column and the
sort's targetlist contains only that column.

This allows us to re-enable Datum sorts for byref types which was disabled
in 3a5817695 due to a reported memory leak.

Additionally, here we slightly optimize DISTINCT aggregates so that we no
longer perform any datumCopy() when we find the current value not to be
distinct from the previous value.  Previously the code would always take a
copy of the most recent Datum and pfree the previous value, even when the
values were the same.  Testing shows a small but noticeable performance
increase when aggregate transitions are skipped due to the current
transition value being the same as the prior one.

Author: David Rowley
Discussion: https://postgr.es/m/CAApHDvqS6wC5U==k9Hd26E4EQXH3QR67-T4=Q1rQ36NGvjfVSg@mail.gmail.com
Discussion: https://postgr.es/m/CAApHDvqHonfe9G1cVaKeHbDx70R_zCrM3qP2AGXpGrieSKGnhA@mail.gmail.com

diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index a3414a76e8d3cc61b6b9c068b916210b6e4d88f2..41f1ca65d0143b29dc4a8cc6d62f0b2b46abd01b 100644 (file)
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -1879,17 +1879,13 @@ heapam_index_validate_scan(Relation heapRelation,
            }
 
            tuplesort_empty = !tuplesort_getdatum(state->tuplesort, true,
-                                                 &ts_val, &ts_isnull, NULL);
+                                                 false, &ts_val, &ts_isnull,
+                                                 NULL);
            Assert(tuplesort_empty || !ts_isnull);
            if (!tuplesort_empty)
            {
                itemptr_decode(&decoded, DatumGetInt64(ts_val));
                indexcursor = &decoded;
-
-               /* If int8 is pass-by-ref, free (encoded) TID Datum memory */
-#ifndef USE_FLOAT8_BYVAL
-               pfree(DatumGetPointer(ts_val));
-#endif
            }
            else
            {

diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index bcf3b1950b1f89aee8647ada544f53f746823775..28f6f9c5c5aa5e60ea895a228c907f49b4a1689a 100644 (file)
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -879,7 +879,7 @@ process_ordered_aggregate_single(AggState *aggstate,
     */
 
    while (tuplesort_getdatum(pertrans->sortstates[aggstate->current_set],
-                             true, newVal, isNull, &newAbbrevVal))
+                             true, false, newVal, isNull, &newAbbrevVal))
    {
        /*
         * Clear and select the working context for evaluation of the equality
@@ -900,24 +900,33 @@ process_ordered_aggregate_single(AggState *aggstate,
                                             pertrans->aggCollation,
                                             oldVal, *newVal)))))
        {
-           /* equal to prior, so forget this one */
-           if (!pertrans->inputtypeByVal && !*isNull)
-               pfree(DatumGetPointer(*newVal));
+           MemoryContextSwitchTo(oldContext);
+           continue;
        }
        else
        {
            advance_transition_function(aggstate, pertrans, pergroupstate);
-           /* forget the old value, if any */
-           if (!oldIsNull && !pertrans->inputtypeByVal)
-               pfree(DatumGetPointer(oldVal));
-           /* and remember the new one for subsequent equality checks */
-           oldVal = *newVal;
+
+           MemoryContextSwitchTo(oldContext);
+
+           /*
+            * Forget the old value, if any, and remember the new one for
+            * subsequent equality checks.
+            */
+           if (!pertrans->inputtypeByVal)
+           {
+               if (!oldIsNull)
+                   pfree(DatumGetPointer(oldVal));
+               if (!*isNull)
+                   oldVal = datumCopy(*newVal, pertrans->inputtypeByVal,
+                                      pertrans->inputtypeLen);
+           }
+           else
+               oldVal = *newVal;
            oldAbbrevVal = newAbbrevVal;
            oldIsNull = *isNull;
            haveOldVal = true;
        }
-
-       MemoryContextSwitchTo(oldContext);
    }
 
    if (!oldIsNull && !pertrans->inputtypeByVal)

diff --git a/src/backend/executor/nodeSort.c b/src/backend/executor/nodeSort.c
index 37ad35704bc49706d242c665179c6e3c0d41bbc4..a8316f0e13c761781b95ff33a95ed554a7a8b322 100644 (file)
--- a/src/backend/executor/nodeSort.c
+++ b/src/backend/executor/nodeSort.c
@@ -198,7 +198,8 @@ ExecSort(PlanState *pstate)
    {
        ExecClearTuple(slot);
        if (tuplesort_getdatum(tuplesortstate, ScanDirectionIsForward(dir),
-                              &(slot->tts_values[0]), &(slot->tts_isnull[0]), NULL))
+                              false, &(slot->tts_values[0]),
+                              &(slot->tts_isnull[0]), NULL))
            ExecStoreVirtualTuple(slot);
    }
    else
@@ -278,10 +279,10 @@ ExecInitSort(Sort *node, EState *estate, int eflags)
    outerTupDesc = ExecGetResultType(outerPlanState(sortstate));
 
    /*
-    * We perform a Datum sort when we're sorting just a single byval column,
+    * We perform a Datum sort when we're sorting just a single column,
     * otherwise we perform a tuple sort.
     */
-   if (outerTupDesc->natts == 1 && TupleDescAttr(outerTupDesc, 0)->attbyval)
+   if (outerTupDesc->natts == 1)
        sortstate->datumSort = true;
    else
        sortstate->datumSort = false;

diff --git a/src/backend/utils/adt/orderedsetaggs.c b/src/backend/utils/adt/orderedsetaggs.c
index 185b2cb848719d2b019efd98d881d7cf02546551..d19e7ba8718c5cc583a12f0ceff25a6481b0ccfe 100644 (file)
--- a/src/backend/utils/adt/orderedsetaggs.c
+++ b/src/backend/utils/adt/orderedsetaggs.c
@@ -482,7 +482,8 @@ percentile_disc_final(PG_FUNCTION_ARGS)
            elog(ERROR, "missing row in percentile_disc");
    }
 
-   if (!tuplesort_getdatum(osastate->sortstate, true, &val, &isnull, NULL))
+   if (!tuplesort_getdatum(osastate->sortstate, true, true, &val, &isnull,
+                           NULL))
        elog(ERROR, "missing row in percentile_disc");
 
    /* We shouldn't have stored any nulls, but do the right thing anyway */
@@ -581,7 +582,8 @@ percentile_cont_final_common(FunctionCallInfo fcinfo,
    if (!tuplesort_skiptuples(osastate->sortstate, first_row, true))
        elog(ERROR, "missing row in percentile_cont");
 
-   if (!tuplesort_getdatum(osastate->sortstate, true, &first_val, &isnull, NULL))
+   if (!tuplesort_getdatum(osastate->sortstate, true, true, &first_val,
+                           &isnull, NULL))
        elog(ERROR, "missing row in percentile_cont");
    if (isnull)
        PG_RETURN_NULL();
@@ -592,7 +594,8 @@ percentile_cont_final_common(FunctionCallInfo fcinfo,
    }
    else
    {
-       if (!tuplesort_getdatum(osastate->sortstate, true, &second_val, &isnull, NULL))
+       if (!tuplesort_getdatum(osastate->sortstate, true, true, &second_val,
+                               &isnull, NULL))
            elog(ERROR, "missing row in percentile_cont");
 
        if (isnull)
@@ -817,7 +820,8 @@ percentile_disc_multi_final(PG_FUNCTION_ARGS)
                if (!tuplesort_skiptuples(osastate->sortstate, target_row - rownum - 1, true))
                    elog(ERROR, "missing row in percentile_disc");
 
-               if (!tuplesort_getdatum(osastate->sortstate, true, &val, &isnull, NULL))
+               if (!tuplesort_getdatum(osastate->sortstate, true, true, &val,
+                                       &isnull, NULL))
                    elog(ERROR, "missing row in percentile_disc");
 
                rownum = target_row;
@@ -945,8 +949,8 @@ percentile_cont_multi_final_common(FunctionCallInfo fcinfo,
                if (!tuplesort_skiptuples(osastate->sortstate, first_row - rownum - 1, true))
                    elog(ERROR, "missing row in percentile_cont");
 
-               if (!tuplesort_getdatum(osastate->sortstate, true, &first_val,
-                                       &isnull, NULL) || isnull)
+               if (!tuplesort_getdatum(osastate->sortstate, true, true,
+                                       &first_val, &isnull, NULL) || isnull)
                    elog(ERROR, "missing row in percentile_cont");
 
                rownum = first_row;
@@ -966,8 +970,8 @@ percentile_cont_multi_final_common(FunctionCallInfo fcinfo,
            /* Fetch second_row if needed */
            if (second_row > rownum)
            {
-               if (!tuplesort_getdatum(osastate->sortstate, true, &second_val,
-                                       &isnull, NULL) || isnull)
+               if (!tuplesort_getdatum(osastate->sortstate, true, true,
+                                       &second_val, &isnull, NULL) || isnull)
                    elog(ERROR, "missing row in percentile_cont");
                rownum++;
            }
@@ -1073,7 +1077,8 @@ mode_final(PG_FUNCTION_ARGS)
        tuplesort_rescan(osastate->sortstate);
 
    /* Scan tuples and count frequencies */
-   while (tuplesort_getdatum(osastate->sortstate, true, &val, &isnull, &abbrev_val))
+   while (tuplesort_getdatum(osastate->sortstate, true, true, &val, &isnull,
+                             &abbrev_val))
    {
        /* we don't expect any nulls, but ignore them if found */
        if (isnull)

diff --git a/src/backend/utils/sort/tuplesortvariants.c b/src/backend/utils/sort/tuplesortvariants.c
index afa5bdbf042a4f19c70a4202b94929a246578cc7..f5d72b4752ee36f71a767f819ee384765065d609 100644 (file)
--- a/src/backend/utils/sort/tuplesortvariants.c
+++ b/src/backend/utils/sort/tuplesortvariants.c
@@ -848,9 +848,19 @@ tuplesort_getindextuple(Tuplesortstate *state, bool forward)
  * determination of "non-equal tuple" based on simple binary inequality.  A
  * NULL value will have a zeroed abbreviated value representation, which caller
  * may rely on in abbreviated inequality check.
+ *
+ * For byref Datums, if copy is true, *val is set to a copy of the Datum
+ * copied into the caller's memory context, so that it will stay valid
+ * regardless of future manipulations of the tuplesort's state (up to and
+ * including deleting the tuplesort).  If copy is false, *val will just be
+ * set to a pointer to the Datum 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.
+ * For byval Datums, the value of the 'copy' parameter has no effect.
+
  */
 bool
-tuplesort_getdatum(Tuplesortstate *state, bool forward,
+tuplesort_getdatum(Tuplesortstate *state, bool forward, bool copy,
                   Datum *val, bool *isNull, Datum *abbrev)
 {
    TuplesortPublic *base = TuplesortstateGetPublic(state);
@@ -879,7 +889,11 @@ tuplesort_getdatum(Tuplesortstate *state, bool forward,
    else
    {
        /* use stup.tuple because stup.datum1 may be an abbreviation */
-       *val = datumCopy(PointerGetDatum(stup.tuple), false, arg->datumTypeLen);
+       if (copy)
+           *val = datumCopy(PointerGetDatum(stup.tuple), false,
+                            arg->datumTypeLen);
+       else
+           *val = PointerGetDatum(stup.tuple);
        *isNull = false;
    }
 

diff --git a/src/include/utils/tuplesort.h b/src/include/utils/tuplesort.h
index 44412749906cc37c5819ea26fc93b66eac83acdf..59fbe2bd35b41a22e0c19b1700cd400565f36b4c 100644 (file)
--- a/src/include/utils/tuplesort.h
+++ b/src/include/utils/tuplesort.h
@@ -439,7 +439,7 @@ extern bool tuplesort_gettupleslot(Tuplesortstate *state, bool forward,
                                   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,
+extern bool tuplesort_getdatum(Tuplesortstate *state, bool forward, bool copy,
                               Datum *val, bool *isNull, Datum *abbrev);