>>>>> "Robert" == Robert Haas <robertmh...@gmail.com> writes:

 >> 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.

 Robert> But this isn't a value of the SQL type "int64".  It's just a
 Robert> bit pattern that has to fit inside however big a Datum happens
 Robert> to be.

It's a bit pattern which is a signed 32-bit or 64-bit integer, computed
in an int32 or int64. Using something other than Int32GetDatum /
Int64GetDatum for it seems unnecessarily surprising.

 >> 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.

 Robert> But all of those are trivial, and the first would have been
 Robert> caught by my compiler if I weren't using such a crappy old
 Robert> compiler.  If anything that might require as much as 10 lines
 Robert> of code churn to fix is not worth doing, very little is worth
 Robert> doing.

Trivial maybe, but subtle enough that you missed them (which suggests
that others might too), and a --disable-float8-byval build of the buggy
version only fails regression by a fluke.

(This does rather suggest to me that some better regression tests for
sorting would be a good idea, possibly even including on-disk sorts.)

 >> 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.

 Robert> Patch for that attached.

That looks reasonable, though I think it could do with a comment
explaining _why_ it's defining its own macros rather than using
Int32*/Int64*. (And I wrote that before seeing Tom's message, even.)

-- 
Andrew (irc:RhodiumToad)


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to