Thanks for looking at this.
On Tue, 19 Apr 2022 at 02:11, John Naylor <john.nay...@enterprisedb.com> wrote:
> IIUC, this function is called by tuplesort_begin_common, which in turn
> is called by tuplesort_begin_{heap, indexes, etc}. The latter callers
> set the onlyKey and now oneKeySort variables as appropriate, and
> sometimes hard-coded to false. Is it intentional to set them here
> first?
>
> Falling under the polish that you were likely thinking of above:
I did put the patch together quickly just for the benchmark and at the
time I was subtly aware that the onlyKey field was being set using a
similar condition as I was using to set the boolean field I'd added.
On reflection today, it should be fine just to check if that field is
NULL or not in the 3 new comparison functions. Similarly to before,
this only needs to be done if the datums compare equally, so does not
add any code to the path where the datums are non-equal. It looks
like the other tuplesort_begin_* functions use a different comparison
function that will never make use of the specialization comparison
functions added by 697492434.
I separated out the "or" condition that I'd added tot he existing "if"
to make it easier to write a comment explaining why we can skip the
tiebreak function call.
Updated patch attached.
David
diff --git a/src/backend/utils/sort/tuplesort.c
b/src/backend/utils/sort/tuplesort.c
index 1174e1a31c..12d1bf551b 100644
--- a/src/backend/utils/sort/tuplesort.c
+++ b/src/backend/utils/sort/tuplesort.c
@@ -436,7 +436,10 @@ struct Tuplesortstate
/*
* This variable is shared by the single-key MinimalTuple case and the
- * Datum case (which both use qsort_ssup()). Otherwise it's NULL.
+ * Datum case (which both use qsort_ssup()). It is also used by various
+ * sort specialization functions when comparing the leading key in a
+ * tiebreak situation to determine if there are any subsequent keys to
+ * sort on. It's otherwise NULL.
*/
SortSupport onlyKey;
@@ -698,7 +701,12 @@ qsort_tuple_unsigned_compare(SortTuple *a, SortTuple *b,
Tuplesortstate *state)
compare = ApplyUnsignedSortComparator(a->datum1, a->isnull1,
b->datum1, b->isnull1,
&state->sortKeys[0]);
- if (compare != 0)
+
+ /*
+ * No need to call the tiebreak function when the datums differ or if
this
+ * is the only key we're sorting on.
+ */
+ if (compare != 0 || state->onlyKey != NULL)
return compare;
return state->comparetup(a, b, state);
@@ -713,7 +721,12 @@ qsort_tuple_signed_compare(SortTuple *a, SortTuple *b,
Tuplesortstate *state)
compare = ApplySignedSortComparator(a->datum1, a->isnull1,
b->datum1, b->isnull1,
&state->sortKeys[0]);
- if (compare != 0)
+
+ /*
+ * No need to call the tiebreak function when the datums differ or if
this
+ * is the only key we're sorting on.
+ */
+ if (compare != 0 || state->onlyKey != NULL)
return compare;
return state->comparetup(a, b, state);
@@ -728,7 +741,12 @@ qsort_tuple_int32_compare(SortTuple *a, SortTuple *b,
Tuplesortstate *state)
compare = ApplyInt32SortComparator(a->datum1, a->isnull1,
b->datum1, b->isnull1,
&state->sortKeys[0]);
- if (compare != 0)
+
+ /*
+ * No need to call the tiebreak function when the datums differ or if
this
+ * is the only key we're sorting on.
+ */
+ if (compare != 0 || state->onlyKey != NULL)
return compare;
return state->comparetup(a, b, state);
