On Fri, Apr 3, 2015 at 1:39 PM, Andrew Gierth <and...@tao11.riddles.org.uk> wrote: > It already does; it changes how int64 values are expected to be stored > in Datum variables. _Everything_ that currently stores an int64 value in > a Datum is affected.
But this isn't a value of the SQL type "int64". It's just a bit pattern that has to fit inside however big a Datum happens to be. > As I see it, you need a really good reason to override that in a > specific case, and supporting 64-bit abbreviations on a > --disable-float8-byval build really isn't a good reason (since 32-bit > abbrevs work fine and such builds should be vanishingly rare anyway). In my opinion, there is way too much crap around already just to cater to --disable-float8-byval. I think not adding more is a perfectly reasonable goal. If somebody wants to remove --disable-float8-byval some day, I want to minimize the amount of stuff they have to change in order to do that. I think that keeping this off the list of stuff that will require modification is a worthy goal. > The fact that making this one low-benefit change has introduced no less > than three separate bugs - the compile error with that #ifdef, the use > of Int64GetDatum for NANs, and the use of Int64GetDatum for the return > value of the abbreviation function should possibly be taken as a hint to > how bad an idea is. But all of those are trivial, and the first would have been caught by my compiler if I weren't using such a crappy old compiler. If anything that might require as much as 10 lines of code churn to fix is not worth doing, very little is worth doing. > If you're determined to go this route - over my protest - then you need > to do something like define a NumericAbbrevGetDatum(x) macro and use it > in place of the Int64GetDatum / Int32GetDatum ones for both NAN and the > return from numeric_abbrev_convert_var. Patch for that attached. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c index dd108c8..0b11985 100644 --- a/src/backend/utils/adt/numeric.c +++ b/src/backend/utils/adt/numeric.c @@ -298,11 +298,13 @@ typedef struct #define NUMERIC_ABBREV_BITS (SIZEOF_DATUM * BITS_PER_BYTE) #if SIZEOF_DATUM == 8 -#define DatumGetNumericAbbrev(d) ((int64) d) -#define NUMERIC_ABBREV_NAN Int64GetDatum(PG_INT64_MIN) +#define NumericAbbrevGetDatum(X) ((Datum) SET_8_BYTES(X)) +#define DatumGetNumericAbbrev(X) ((int64) GET_8_BYTES(X)) +#define NUMERIC_ABBREV_NAN NumericAbbrevGetDatum(PG_INT64_MIN) #else -#define DatumGetNumericAbbrev(d) ((int32) d) -#define NUMERIC_ABBREV_NAN Int32GetDatum(PG_INT32_MIN) +#define NumericAbbrevGetDatum(X) ((Datum) SET_4_BYTES(X)) +#define DatumGetNumericAbbrev(X) ((int32) GET_4_BYTES(X)) +#define NUMERIC_ABBREV_NAN NumericAbbrevGetDatum(PG_INT32_MIN) #endif @@ -1883,7 +1885,7 @@ numeric_abbrev_convert_var(NumericVar *var, NumericSortSupport *nss) addHyperLogLog(&nss->abbr_card, DatumGetUInt32(hash_uint32(tmp))); } - return Int64GetDatum(result); + return NumericAbbrevGetDatum(result); } #endif /* NUMERIC_ABBREV_BITS == 64 */ @@ -1960,7 +1962,7 @@ numeric_abbrev_convert_var(NumericVar *var, NumericSortSupport *nss) addHyperLogLog(&nss->abbr_card, DatumGetUInt32(hash_uint32(tmp))); } - return Int32GetDatum(result); + return NumericAbbrevGetDatum(result); } #endif /* NUMERIC_ABBREV_BITS == 32 */
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
