Skip to content

Commit cc58eec

Browse files
macdicemacdice
committed
Fix tuplesort optimization for CLUSTER-on-expression.
When dispatching sort operations to specialized variants, commit 6974924 failed to handle the case where CLUSTER-sort decides not to initialize datum1 and isnull1. Fix by hoisting that decision up a level and advertising whether datum1 can be relied on, in the Tuplesortstate object. Per reports from UBsan and Valgrind build farm animals, while running the cluster.sql test. Reviewed-by: Andres Freund <andres@anarazel.de> Discussion: https://postgr.es/m/CAFBsxsF1TeK5Fic0M%2BTSJXzbKsY6aBqJGNj6ptURuB09ZF6k_w%40mail.gmail.com
1 parent 591e088 commit cc58eec

File tree

1 file changed

+56
-22
lines changed

1 file changed

+56
-22
lines changed

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

+56-22
Original file line numberDiff line numberDiff line change
@@ -306,6 +306,12 @@ struct Tuplesortstate
306306
void (*readtup) (Tuplesortstate *state, SortTuple *stup,
307307
LogicalTape *tape, unsigned int len);
308308

309+
/*
310+
* Whether SortTuple's datum1 and isnull1 members are maintained by the
311+
* above routines. If not, some sort specializations are disabled.
312+
*/
313+
bool haveDatum1;
314+
309315
/*
310316
* This array holds the tuples now in sort memory. If we are in state
311317
* INITIAL, the tuples are in no particular order; if we are in state
@@ -1016,6 +1022,7 @@ tuplesort_begin_heap(TupleDesc tupDesc,
10161022
state->copytup = copytup_heap;
10171023
state->writetup = writetup_heap;
10181024
state->readtup = readtup_heap;
1025+
state->haveDatum1 = true;
10191026

10201027
state->tupDesc = tupDesc; /* assume we need not copy tupDesc */
10211028
state->abbrevNext = 10;
@@ -1095,6 +1102,15 @@ tuplesort_begin_cluster(TupleDesc tupDesc,
10951102

10961103
state->indexInfo = BuildIndexInfo(indexRel);
10971104

1105+
/*
1106+
* If we don't have a simple leading attribute, we don't currently
1107+
* initialize datum1, so disable optimizations that require it.
1108+
*/
1109+
if (state->indexInfo->ii_IndexAttrNumbers[0] == 0)
1110+
state->haveDatum1 = false;
1111+
else
1112+
state->haveDatum1 = true;
1113+
10981114
state->tupDesc = tupDesc; /* assume we need not copy tupDesc */
10991115

11001116
indexScanKey = _bt_mkscankey(indexRel, NULL);
@@ -1188,6 +1204,7 @@ tuplesort_begin_index_btree(Relation heapRel,
11881204
state->writetup = writetup_index;
11891205
state->readtup = readtup_index;
11901206
state->abbrevNext = 10;
1207+
state->haveDatum1 = true;
11911208

11921209
state->heapRel = heapRel;
11931210
state->indexRel = indexRel;
@@ -1262,6 +1279,7 @@ tuplesort_begin_index_hash(Relation heapRel,
12621279
state->copytup = copytup_index;
12631280
state->writetup = writetup_index;
12641281
state->readtup = readtup_index;
1282+
state->haveDatum1 = true;
12651283

12661284
state->heapRel = heapRel;
12671285
state->indexRel = indexRel;
@@ -1302,6 +1320,7 @@ tuplesort_begin_index_gist(Relation heapRel,
13021320
state->copytup = copytup_index;
13031321
state->writetup = writetup_index;
13041322
state->readtup = readtup_index;
1323+
state->haveDatum1 = true;
13051324

13061325
state->heapRel = heapRel;
13071326
state->indexRel = indexRel;
@@ -1366,6 +1385,7 @@ tuplesort_begin_datum(Oid datumType, Oid sortOperator, Oid sortCollation,
13661385
state->writetup = writetup_datum;
13671386
state->readtup = readtup_datum;
13681387
state->abbrevNext = 10;
1388+
state->haveDatum1 = true;
13691389

13701390
state->datumType = datumType;
13711391

@@ -3593,27 +3613,40 @@ tuplesort_sort_memtuples(Tuplesortstate *state)
35933613

35943614
if (state->memtupcount > 1)
35953615
{
3596-
/* Do we have a specialization for the leading column's comparator? */
3597-
if (state->sortKeys &&
3598-
state->sortKeys[0].comparator == ssup_datum_unsigned_cmp)
3599-
{
3600-
elog(DEBUG1, "qsort_tuple_unsigned");
3601-
qsort_tuple_unsigned(state->memtuples, state->memtupcount, state);
3602-
}
3603-
else if (state->sortKeys &&
3604-
state->sortKeys[0].comparator == ssup_datum_signed_cmp)
3605-
{
3606-
elog(DEBUG1, "qsort_tuple_signed");
3607-
qsort_tuple_signed(state->memtuples, state->memtupcount, state);
3608-
}
3609-
else if (state->sortKeys &&
3610-
state->sortKeys[0].comparator == ssup_datum_int32_cmp)
3616+
/*
3617+
* Do we have the leading column's value or abbreviation in datum1,
3618+
* and is there a specialization for its comparator?
3619+
*/
3620+
if (state->haveDatum1 && state->sortKeys)
36113621
{
3612-
elog(DEBUG1, "qsort_tuple_int32");
3613-
qsort_tuple_int32(state->memtuples, state->memtupcount, state);
3622+
if (state->sortKeys[0].comparator == ssup_datum_unsigned_cmp)
3623+
{
3624+
elog(DEBUG1, "qsort_tuple_unsigned");
3625+
qsort_tuple_unsigned(state->memtuples,
3626+
state->memtupcount,
3627+
state);
3628+
return;
3629+
}
3630+
else if (state->sortKeys[0].comparator == ssup_datum_signed_cmp)
3631+
{
3632+
elog(DEBUG1, "qsort_tuple_signed");
3633+
qsort_tuple_signed(state->memtuples,
3634+
state->memtupcount,
3635+
state);
3636+
return;
3637+
}
3638+
else if (state->sortKeys[0].comparator == ssup_datum_int32_cmp)
3639+
{
3640+
elog(DEBUG1, "qsort_tuple_int32");
3641+
qsort_tuple_int32(state->memtuples,
3642+
state->memtupcount,
3643+
state);
3644+
return;
3645+
}
36143646
}
3647+
36153648
/* Can we use the single-key sort function? */
3616-
else if (state->onlyKey != NULL)
3649+
if (state->onlyKey != NULL)
36173650
{
36183651
elog(DEBUG1, "qsort_ssup");
36193652
qsort_ssup(state->memtuples, state->memtupcount,
@@ -4019,15 +4052,14 @@ comparetup_cluster(const SortTuple *a, const SortTuple *b,
40194052
datum2;
40204053
bool isnull1,
40214054
isnull2;
4022-
AttrNumber leading = state->indexInfo->ii_IndexAttrNumbers[0];
40234055

40244056
/* Be prepared to compare additional sort keys */
40254057
ltup = (HeapTuple) a->tuple;
40264058
rtup = (HeapTuple) b->tuple;
40274059
tupDesc = state->tupDesc;
40284060

40294061
/* Compare the leading sort key, if it's simple */
4030-
if (leading != 0)
4062+
if (state->haveDatum1)
40314063
{
40324064
compare = ApplySortComparator(a->datum1, a->isnull1,
40334065
b->datum1, b->isnull1,
@@ -4037,6 +4069,8 @@ comparetup_cluster(const SortTuple *a, const SortTuple *b,
40374069

40384070
if (sortKey->abbrev_converter)
40394071
{
4072+
AttrNumber leading = state->indexInfo->ii_IndexAttrNumbers[0];
4073+
40404074
datum1 = heap_getattr(ltup, leading, tupDesc, &isnull1);
40414075
datum2 = heap_getattr(rtup, leading, tupDesc, &isnull2);
40424076

@@ -4134,7 +4168,7 @@ copytup_cluster(Tuplesortstate *state, SortTuple *stup, void *tup)
41344168
* set up first-column key value, and potentially abbreviate, if it's a
41354169
* simple column
41364170
*/
4137-
if (state->indexInfo->ii_IndexAttrNumbers[0] == 0)
4171+
if (!state->haveDatum1)
41384172
return;
41394173

41404174
original = heap_getattr(tuple,
@@ -4229,7 +4263,7 @@ readtup_cluster(Tuplesortstate *state, SortTuple *stup,
42294263
LogicalTapeReadExact(tape, &tuplen, sizeof(tuplen));
42304264
stup->tuple = (void *) tuple;
42314265
/* set up first-column key value, if it's a simple column */
4232-
if (state->indexInfo->ii_IndexAttrNumbers[0] != 0)
4266+
if (state->haveDatum1)
42334267
stup->datum1 = heap_getattr(tuple,
42344268
state->indexInfo->ii_IndexAttrNumbers[0],
42354269
state->tupDesc,

0 commit comments

Comments
 (0)