Skip to content

Commit d37aa3d

Browse files
david-rowleydavid-rowley
committed
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 91e9e89, 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 3a58176 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
1 parent a5fc464 commit d37aa3d

File tree

6 files changed

+57
-32
lines changed

6 files changed

+57
-32
lines changed

‎src/backend/access/heap/heapam_handler.c

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1879,17 +1879,13 @@ heapam_index_validate_scan(Relation heapRelation,
18791879
}
18801880

18811881
tuplesort_empty = !tuplesort_getdatum(state->tuplesort, true,
1882-
&ts_val, &ts_isnull, NULL);
1882+
false, &ts_val, &ts_isnull,
1883+
NULL);
18831884
Assert(tuplesort_empty || !ts_isnull);
18841885
if (!tuplesort_empty)
18851886
{
18861887
itemptr_decode(&decoded, DatumGetInt64(ts_val));
18871888
indexcursor = &decoded;
1888-
1889-
/* If int8 is pass-by-ref, free (encoded) TID Datum memory */
1890-
#ifndef USE_FLOAT8_BYVAL
1891-
pfree(DatumGetPointer(ts_val));
1892-
#endif
18931889
}
18941890
else
18951891
{

‎src/backend/executor/nodeAgg.c

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -879,7 +879,7 @@ process_ordered_aggregate_single(AggState *aggstate,
879879
*/
880880

881881
while (tuplesort_getdatum(pertrans->sortstates[aggstate->current_set],
882-
true, newVal, isNull, &newAbbrevVal))
882+
true, false, newVal, isNull, &newAbbrevVal))
883883
{
884884
/*
885885
* Clear and select the working context for evaluation of the equality
@@ -900,24 +900,33 @@ process_ordered_aggregate_single(AggState *aggstate,
900900
pertrans->aggCollation,
901901
oldVal, *newVal)))))
902902
{
903-
/* equal to prior, so forget this one */
904-
if (!pertrans->inputtypeByVal && !*isNull)
905-
pfree(DatumGetPointer(*newVal));
903+
MemoryContextSwitchTo(oldContext);
904+
continue;
906905
}
907906
else
908907
{
909908
advance_transition_function(aggstate, pertrans, pergroupstate);
910-
/* forget the old value, if any */
911-
if (!oldIsNull && !pertrans->inputtypeByVal)
912-
pfree(DatumGetPointer(oldVal));
913-
/* and remember the new one for subsequent equality checks */
914-
oldVal = *newVal;
909+
910+
MemoryContextSwitchTo(oldContext);
911+
912+
/*
913+
* Forget the old value, if any, and remember the new one for
914+
* subsequent equality checks.
915+
*/
916+
if (!pertrans->inputtypeByVal)
917+
{
918+
if (!oldIsNull)
919+
pfree(DatumGetPointer(oldVal));
920+
if (!*isNull)
921+
oldVal = datumCopy(*newVal, pertrans->inputtypeByVal,
922+
pertrans->inputtypeLen);
923+
}
924+
else
925+
oldVal = *newVal;
915926
oldAbbrevVal = newAbbrevVal;
916927
oldIsNull = *isNull;
917928
haveOldVal = true;
918929
}
919-
920-
MemoryContextSwitchTo(oldContext);
921930
}
922931

923932
if (!oldIsNull && !pertrans->inputtypeByVal)

‎src/backend/executor/nodeSort.c

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -198,7 +198,8 @@ ExecSort(PlanState *pstate)
198198
{
199199
ExecClearTuple(slot);
200200
if (tuplesort_getdatum(tuplesortstate, ScanDirectionIsForward(dir),
201-
&(slot->tts_values[0]), &(slot->tts_isnull[0]), NULL))
201+
false, &(slot->tts_values[0]),
202+
&(slot->tts_isnull[0]), NULL))
202203
ExecStoreVirtualTuple(slot);
203204
}
204205
else
@@ -278,10 +279,10 @@ ExecInitSort(Sort *node, EState *estate, int eflags)
278279
outerTupDesc = ExecGetResultType(outerPlanState(sortstate));
279280

280281
/*
281-
* We perform a Datum sort when we're sorting just a single byval column,
282+
* We perform a Datum sort when we're sorting just a single column,
282283
* otherwise we perform a tuple sort.
283284
*/
284-
if (outerTupDesc->natts == 1 && TupleDescAttr(outerTupDesc, 0)->attbyval)
285+
if (outerTupDesc->natts == 1)
285286
sortstate->datumSort = true;
286287
else
287288
sortstate->datumSort = false;

‎src/backend/utils/adt/orderedsetaggs.c

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -482,7 +482,8 @@ percentile_disc_final(PG_FUNCTION_ARGS)
482482
elog(ERROR, "missing row in percentile_disc");
483483
}
484484

485-
if (!tuplesort_getdatum(osastate->sortstate, true, &val, &isnull, NULL))
485+
if (!tuplesort_getdatum(osastate->sortstate, true, true, &val, &isnull,
486+
NULL))
486487
elog(ERROR, "missing row in percentile_disc");
487488

488489
/* We shouldn't have stored any nulls, but do the right thing anyway */
@@ -581,7 +582,8 @@ percentile_cont_final_common(FunctionCallInfo fcinfo,
581582
if (!tuplesort_skiptuples(osastate->sortstate, first_row, true))
582583
elog(ERROR, "missing row in percentile_cont");
583584

584-
if (!tuplesort_getdatum(osastate->sortstate, true, &first_val, &isnull, NULL))
585+
if (!tuplesort_getdatum(osastate->sortstate, true, true, &first_val,
586+
&isnull, NULL))
585587
elog(ERROR, "missing row in percentile_cont");
586588
if (isnull)
587589
PG_RETURN_NULL();
@@ -592,7 +594,8 @@ percentile_cont_final_common(FunctionCallInfo fcinfo,
592594
}
593595
else
594596
{
595-
if (!tuplesort_getdatum(osastate->sortstate, true, &second_val, &isnull, NULL))
597+
if (!tuplesort_getdatum(osastate->sortstate, true, true, &second_val,
598+
&isnull, NULL))
596599
elog(ERROR, "missing row in percentile_cont");
597600

598601
if (isnull)
@@ -817,7 +820,8 @@ percentile_disc_multi_final(PG_FUNCTION_ARGS)
817820
if (!tuplesort_skiptuples(osastate->sortstate, target_row - rownum - 1, true))
818821
elog(ERROR, "missing row in percentile_disc");
819822

820-
if (!tuplesort_getdatum(osastate->sortstate, true, &val, &isnull, NULL))
823+
if (!tuplesort_getdatum(osastate->sortstate, true, true, &val,
824+
&isnull, NULL))
821825
elog(ERROR, "missing row in percentile_disc");
822826

823827
rownum = target_row;
@@ -945,8 +949,8 @@ percentile_cont_multi_final_common(FunctionCallInfo fcinfo,
945949
if (!tuplesort_skiptuples(osastate->sortstate, first_row - rownum - 1, true))
946950
elog(ERROR, "missing row in percentile_cont");
947951

948-
if (!tuplesort_getdatum(osastate->sortstate, true, &first_val,
949-
&isnull, NULL) || isnull)
952+
if (!tuplesort_getdatum(osastate->sortstate, true, true,
953+
&first_val, &isnull, NULL) || isnull)
950954
elog(ERROR, "missing row in percentile_cont");
951955

952956
rownum = first_row;
@@ -966,8 +970,8 @@ percentile_cont_multi_final_common(FunctionCallInfo fcinfo,
966970
/* Fetch second_row if needed */
967971
if (second_row > rownum)
968972
{
969-
if (!tuplesort_getdatum(osastate->sortstate, true, &second_val,
970-
&isnull, NULL) || isnull)
973+
if (!tuplesort_getdatum(osastate->sortstate, true, true,
974+
&second_val, &isnull, NULL) || isnull)
971975
elog(ERROR, "missing row in percentile_cont");
972976
rownum++;
973977
}
@@ -1073,7 +1077,8 @@ mode_final(PG_FUNCTION_ARGS)
10731077
tuplesort_rescan(osastate->sortstate);
10741078

10751079
/* Scan tuples and count frequencies */
1076-
while (tuplesort_getdatum(osastate->sortstate, true, &val, &isnull, &abbrev_val))
1080+
while (tuplesort_getdatum(osastate->sortstate, true, true, &val, &isnull,
1081+
&abbrev_val))
10771082
{
10781083
/* we don't expect any nulls, but ignore them if found */
10791084
if (isnull)

‎src/backend/utils/sort/tuplesortvariants.c

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -848,9 +848,19 @@ tuplesort_getindextuple(Tuplesortstate *state, bool forward)
848848
* determination of "non-equal tuple" based on simple binary inequality. A
849849
* NULL value will have a zeroed abbreviated value representation, which caller
850850
* may rely on in abbreviated inequality check.
851+
*
852+
* For byref Datums, if copy is true, *val is set to a copy of the Datum
853+
* copied into the caller's memory context, so that it will stay valid
854+
* regardless of future manipulations of the tuplesort's state (up to and
855+
* including deleting the tuplesort). If copy is false, *val will just be
856+
* set to a pointer to the Datum held within the tuplesort, which is more
857+
* efficient, but only safe for callers that are prepared to have any
858+
* subsequent manipulation of the tuplesort's state invalidate slot contents.
859+
* For byval Datums, the value of the 'copy' parameter has no effect.
860+
851861
*/
852862
bool
853-
tuplesort_getdatum(Tuplesortstate *state, bool forward,
863+
tuplesort_getdatum(Tuplesortstate *state, bool forward, bool copy,
854864
Datum *val, bool *isNull, Datum *abbrev)
855865
{
856866
TuplesortPublic *base = TuplesortstateGetPublic(state);
@@ -879,7 +889,11 @@ tuplesort_getdatum(Tuplesortstate *state, bool forward,
879889
else
880890
{
881891
/* use stup.tuple because stup.datum1 may be an abbreviation */
882-
*val = datumCopy(PointerGetDatum(stup.tuple), false, arg->datumTypeLen);
892+
if (copy)
893+
*val = datumCopy(PointerGetDatum(stup.tuple), false,
894+
arg->datumTypeLen);
895+
else
896+
*val = PointerGetDatum(stup.tuple);
883897
*isNull = false;
884898
}
885899

‎src/include/utils/tuplesort.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -439,7 +439,7 @@ extern bool tuplesort_gettupleslot(Tuplesortstate *state, bool forward,
439439
bool copy, TupleTableSlot *slot, Datum *abbrev);
440440
extern HeapTuple tuplesort_getheaptuple(Tuplesortstate *state, bool forward);
441441
extern IndexTuple tuplesort_getindextuple(Tuplesortstate *state, bool forward);
442-
extern bool tuplesort_getdatum(Tuplesortstate *state, bool forward,
442+
extern bool tuplesort_getdatum(Tuplesortstate *state, bool forward, bool copy,
443443
Datum *val, bool *isNull, Datum *abbrev);
444444

445445

0 commit comments

Comments
 (0)