On Tue, May 10, 2022 at 3:46 PM David Rowley <dgrowleyml@gmail.com> wrote:
> 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?
Right.
> 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.
Testing for Datum size seems more in line with the intent. (Even aside
from that hypothetical, making all Datums 8 bytes has been suggested
before, and this might make that change easier.)
> 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?
+1
--
John Naylor
EDB: http://www.enterprisedb.com