Fix some incorrect preprocessor tests in tuplesort specializations

david-rowley · david-rowley · commit c90c16591c43 · 2022-05-11T11:38:13.000+12:00
6974924 added 3 new quicksort specialization functions for common datatypes. That commit was not very consistent in how it would determine if we're compiling for 32-bit or 64-bit machines. It would sometimes use USE_FLOAT8_BYVAL and at other times check if SIZEOF_DATUM == 8. This could cause theoretical problems due to the way USE_FLOAT8_BYVAL is now defined based on SIZEOF_VOID_P >= 8. If pointers for some reason were ever larger than 8-bytes then we'd end up doing 32-bit comparisons mistakenly. Let's just always check SIZEOF_DATUM >= 8. It also seems that ssup_datum_signed_cmp is just never used on 32-bit builds, so let's just ifdef that out to make sure we never accidentally use that comparison function on such machines. This also allows us to ifdef out 1 of the 3 new specialization quicksort functions in 32-bit builds which seems to shrink down the binary by over 4KB on my machine. In passing, also add the missing DatumGetInt32() / DatumGetInt64() macros in the comparison functions. Discussion: https://postgr.es/m/CAApHDvqcQExRhtRa9hJrJB_5egs3SUfOcutP3m+3HO8A+fZTPA@mail.gmail.com Reviewed-by: John Naylor
diff --git a/src/backend/access/nbtree/nbtcompare.c b/src/backend/access/nbtree/nbtcompare.c
@@ -142,7 +142,7 @@ btint8cmp(PG_FUNCTION_ARGS)
 		PG_RETURN_INT32(A_LESS_THAN_B);
 }
 
-#ifndef USE_FLOAT8_BYVAL
+#if SIZEOF_DATUM < 8
 static int
 btint8fastcmp(Datum x, Datum y, SortSupport ssup)
 {
@@ -163,7 +163,7 @@ btint8sortsupport(PG_FUNCTION_ARGS)
 {
 	SortSupport ssup = (SortSupport) PG_GETARG_POINTER(0);
 
-#ifdef USE_FLOAT8_BYVAL
+#if SIZEOF_DATUM >= 8
 	ssup->comparator = ssup_datum_signed_cmp;
 #else
 	ssup->comparator = btint8fastcmp;
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
@@ -2176,7 +2176,7 @@ timestamp_cmp(PG_FUNCTION_ARGS)
 	PG_RETURN_INT32(timestamp_cmp_internal(dt1, dt2));
 }
 
-#ifndef USE_FLOAT8_BYVAL
+#if SIZEOF_DATUM < 8
 /* note: this is used for timestamptz also */
 static int
 timestamp_fastcmp(Datum x, Datum y, SortSupport ssup)
@@ -2193,7 +2193,7 @@ timestamp_sortsupport(PG_FUNCTION_ARGS)
 {
 	SortSupport ssup = (SortSupport) PG_GETARG_POINTER(0);
 
-#ifdef USE_FLOAT8_BYVAL
+#if SIZEOF_DATUM >= 8
 	/*
 	 * If this build has pass-by-value timestamps, then we can use a standard
 	 * comparator function.
diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c
@@ -715,6 +715,7 @@ qsort_tuple_unsigned_compare(SortTuple *a, SortTuple *b, Tuplesortstate *state)
 	return state->comparetup(a, b, state);
 }
 
+#if SIZEOF_DATUM >= 8
 /* Used if first key's comparator is ssup_datum_signed_compare */
 static pg_attribute_always_inline int
 qsort_tuple_signed_compare(SortTuple *a, SortTuple *b, Tuplesortstate *state)
@@ -737,6 +738,7 @@ qsort_tuple_signed_compare(SortTuple *a, SortTuple *b, Tuplesortstate *state)
 
 	return state->comparetup(a, b, state);
 }
+#endif
 
 /* Used if first key's comparator is ssup_datum_int32_compare */
 static pg_attribute_always_inline int
@@ -779,6 +781,7 @@ qsort_tuple_int32_compare(SortTuple *a, SortTuple *b, Tuplesortstate *state)
 #define ST_DEFINE
 #include "lib/sort_template.h"
 
+#if SIZEOF_DATUM >= 8
 #define ST_SORT qsort_tuple_signed
 #define ST_ELEMENT_TYPE SortTuple
 #define ST_COMPARE(a, b, state) qsort_tuple_signed_compare(a, b, state)
@@ -787,6 +790,7 @@ qsort_tuple_int32_compare(SortTuple *a, SortTuple *b, Tuplesortstate *state)
 #define ST_SCOPE static
 #define ST_DEFINE
 #include "lib/sort_template.h"
+#endif
 
 #define ST_SORT qsort_tuple_int32
 #define ST_ELEMENT_TYPE SortTuple
@@ -3666,6 +3670,7 @@ tuplesort_sort_memtuples(Tuplesortstate *state)
 									 state);
 				return;
 			}
+#if SIZEOF_DATUM >= 8
 			else if (state->sortKeys[0].comparator == ssup_datum_signed_cmp)
 			{
 				elog(DEBUG1, "qsort_tuple_signed");
@@ -3674,6 +3679,7 @@ tuplesort_sort_memtuples(Tuplesortstate *state)
 								   state);
 				return;
 			}
+#endif
 			else if (state->sortKeys[0].comparator == ssup_datum_int32_cmp)
 			{
 				elog(DEBUG1, "qsort_tuple_int32");
@@ -4905,16 +4911,12 @@ ssup_datum_unsigned_cmp(Datum x, Datum y, SortSupport ssup)
 		return 0;
 }
 
+#if SIZEOF_DATUM >= 8
 int
 ssup_datum_signed_cmp(Datum x, Datum y, SortSupport ssup)
 {
-#if SIZEOF_DATUM == 8
-	int64		xx = (int64) x;
-	int64		yy = (int64) y;
-#else
-	int32		xx = (int32) x;
-	int32		yy = (int32) y;
-#endif
+	int64		xx = DatumGetInt64(x);
+	int64		yy = DatumGetInt64(y);
 
 	if (xx < yy)
 		return -1;
@@ -4923,12 +4925,13 @@ ssup_datum_signed_cmp(Datum x, Datum y, SortSupport ssup)
 	else
 		return 0;
 }
+#endif
 
 int
 ssup_datum_int32_cmp(Datum x, Datum y, SortSupport ssup)
 {
-	int32		xx = (int32) x;
-	int32		yy = (int32) y;
+	int32		xx = DatumGetInt32(x);
+	int32		yy = DatumGetInt32(y);
 
 	if (xx < yy)
 		return -1;
diff --git a/src/include/utils/sortsupport.h b/src/include/utils/sortsupport.h
@@ -379,7 +379,9 @@ ApplySortAbbrevFullComparator(Datum datum1, bool isNull1,
  * are eligible for faster sorting.
  */
 extern int ssup_datum_unsigned_cmp(Datum x, Datum y, SortSupport ssup);
+#if SIZEOF_DATUM >= 8
 extern int ssup_datum_signed_cmp(Datum x, Datum y, SortSupport ssup);
+#endif
 extern int ssup_datum_int32_cmp(Datum x, Datum y, SortSupport ssup);
 
 /* Other functions in utils/sort/sortsupport.c */

Original file line number	Diff line number	Diff line change
`@@ -142,7 +142,7 @@ btint8cmp(PG_FUNCTION_ARGS)`
`142`	`142`	`PG_RETURN_INT32(A_LESS_THAN_B);`
`143`	`143`	`}`
`144`	`144`
`145`		`-#ifndef USE_FLOAT8_BYVAL`
	`145`	`+#if SIZEOF_DATUM < 8`
`146`	`146`	`static int`
`147`	`147`	`btint8fastcmp(Datum x, Datum y, SortSupport ssup)`
`148`	`148`	`{`
`@@ -163,7 +163,7 @@ btint8sortsupport(PG_FUNCTION_ARGS)`
`163`	`163`	`{`
`164`	`164`	`SortSupport ssup = (SortSupport) PG_GETARG_POINTER(0);`
`165`	`165`
`166`		`-#ifdef USE_FLOAT8_BYVAL`
	`166`	`+#if SIZEOF_DATUM >= 8`
`167`	`167`	`ssup->comparator = ssup_datum_signed_cmp;`
`168`	`168`	`#else`
`169`	`169`	`ssup->comparator = btint8fastcmp;`