Re: pgsql: Specialize tuplesort routines for different kinds of abbreviated

On Sat, 2 Apr 2022 at 21:30, John Naylor <john(dot)naylor(at)postgresql(dot)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