On Sat, 2 Apr 2022 at 21:30, John Naylor <[email protected]> wrote:
> src/backend/utils/sort/tuplesort.c | 160 ++++++++++++++++++++++++++++++++-
I was just looking over this change and wondered a few things:
1. Shouldn't ssup_datum_signed_cmp and ssup_datum_int32_cmp be using
DatumGetInt32 and DatumGetInt64?
2. I also feel that it's relevant to comment about 32-bit safety for
usages of these functions.
I'm trying to work out what the point of ssup_datum_signed_cmp is when
SIZEOF_DATUM == 4. As far as I see, we just never use it. However,
looking at btint8sortsupport(), we seem to set the comparator based on
USE_FLOAT8_BYVAL rather than SIZEOF_DATUM. It's true in master that
USE_FLOAT8_BYVAL will be 1 if SIZEOF_VOIDP >= 8, per:
#if SIZEOF_VOID_P >= 8
#define USE_FLOAT8_BYVAL 1
#endif
#define SIZEOF_DATUM SIZEOF_VOID_P
This is hypothetical, but if for some reason SIZEOF_VOIDP was larger
than 8, say 16, then the above would define USE_FLOAT8_BYVAL resulting
timestamp and bigint using the new comparators. However, the code
you've added to ssup_datum_signed_cmp checks for SIZEOF_DATUM == 8. It
would assume 32-bit accidentally. That would cause issues.
>From what I can see, ssup_datum_signed_cmp just shouldn't exist in
32-bit builds and we should be consistent about how we determine when
comparators to use.
I've attached a patch which is along the lines of how I imagined this
should look.
What do you think?
David
diff --git a/src/backend/access/nbtree/nbtcompare.c
b/src/backend/access/nbtree/nbtcompare.c
index 40de3878fe..7e18e2fc62 100644
--- 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
index 93c10e1ae4..5cf97e8174 100644
--- 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
index e8da988a73..ebf15393d2 100644
--- a/src/backend/utils/sort/tuplesort.c
+++ b/src/backend/utils/sort/tuplesort.c
@@ -3666,6 +3666,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 +3675,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 +4907,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 +4921,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
index 4f7c73f0aa..ae8f4852a8 100644
--- 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 */
