On Sat, 2 Apr 2022 at 21:30, John Naylor <john.naylor@postgresql.org> 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
