>>>>> "Robert" == Robert Haas <robertmhaas@gmail.com> writes:
>> It already does; it changes how int64 values are expected to be>> stored in Datum variables. _Everything_ that
currentlystores an>> int64 value in a Datum is affected.
Robert> But this isn't a value of the SQL type "int64". It's just aRobert> bit pattern that has to fit inside however
biga Datum happensRobert> 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
errorwith that #ifdef,>> the use of Int64GetDatum for NANs, and the use of Int64GetDatum for>> the return value of the
abbreviationfunction 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 beenRobert> caught by my compiler if I weren't using
sucha crappy oldRobert> compiler. If anything that might require as much as 10 linesRobert> of code churn to fix is
notworth doing, very little is worthRobert> 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
returnfrom 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)