On Thu, 29 Sept 2022 at 18:12, David Rowley <dgrowle...@gmail.com> wrote:
> In the attached patch, I've added a function named
> tuplesort_getdatum_nocopy() which is the same as tuplesort_getdatum()
> only without the datumCopy(). I opted for the new function rather than
> a new parameter in the existing function just to reduce branching and
> additional needless overhead.
Per what was said over on [1], I've adjusted the patch to just add a
'copy' parameter to tuplesort_getdatum() instead of adding the
tuplesort_getdatum_nocopy() function.
I also adjusted some code in heapam_index_validate_scan() to pass
copy=false to tuplesort_getdatum(). The datum in question here is a
TID type, so this really only saves a datumCopy() / pfree on 32-bit
systems. I wasn't too interested in speeding 32-bit systems up with
this, it was more a case of being able to remove the #ifndef
USE_FLOAT8_BYVAL / pfree code.
I think this is a fairly trivial patch, so if nobody objects, I plan
to push it in the next few days.
David
[1] https://www.postgresql.org/message-id/65629.1664460603%40sss.pgh.pa.us
diff --git a/src/backend/access/heap/heapam_handler.c
b/src/backend/access/heap/heapam_handler.c
index a3414a76e8..41f1ca65d0 100644
--- 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 bcf3b1950b..986f761e87 100644
--- 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 37ad35704b..a8316f0e13 100644
--- 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 185b2cb848..d19e7ba871 100644
--- 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 afa5bdbf04..f5d72b4752 100644
--- 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 4441274990..59fbe2bd35 100644
--- 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);
