Обсуждение: [HACKERS] Patch: Avoid precision error in to_timestamp().

Erik Nordström <erik.nordstrom@gmail.com> writes:
> I stumbled upon a precision issue with the to_timestamp() function that
> causes it to return unexpected timestamp values. For instance, the query
> SELECT to_timestamp(1486480176.236538) returns the timestamp "2017-02-07
> 16:09:36.236537+01", which is off by one microsecond. Looking at the source
> code, the issue seems to be that the conversion is unnecessarily done using
> imprecise floating point calculations. Since the target timestamp has
> microsecond precision, and is internally represented by a 64-bit integer
> (on modern platforms), it is better to first convert the given floating
> point value to a microsecond integer and then doing the epoch conversion,
> rather than doing the conversion using floating point and finally casting
> to an integer/timestamp.

This change would introduce overflow failures near the end of the range of
valid inputs.  Maybe it's worth doing anyway and we should just tighten
the range bound tests right above what you patched, but I'm a bit
skeptical.  Float inputs are going to be inherently imprecise anyhow.

I wonder if we could make things better just by using rint() rather than
a naive cast-to-integer.  The cast will truncate not round, and I think
that might be what's mostly biting you.  Does this help for you?

#ifdef HAVE_INT64_TIMESTAMP
-        result = seconds * USECS_PER_SEC;
+        result = rint(seconds * USECS_PER_SEC);
#else
        regards, tom lane

I wrote:
> I wonder if we could make things better just by using rint() rather than
> a naive cast-to-integer.  The cast will truncate not round, and I think
> that might be what's mostly biting you.  Does this help for you?

> #ifdef HAVE_INT64_TIMESTAMP
> -        result = seconds * USECS_PER_SEC;
> +        result = rint(seconds * USECS_PER_SEC);
> #else

I poked around looking for possible similar issues elsewhere, and found
that a substantial majority of the datetime-related code already uses
rint() when trying to go from float to int representations.  As far as
I can find, this function and make_interval() are the only ones that
fail to do so.  So I'm now thinking that this is a clear oversight,
and both those places need to be patched to use rint().
        regards, tom lane

Erik Nordström <erik.nordstrom@gmail.com> writes:
> Thanks for the insightful feedback. You are right, the patch does suffer
> from overflow (and other possible issues when I think about it). Using
> rint(), as you suggest, helps in my original example case, but there are
> still cases when the output is not what you would expect. For instance,
> converting the Unix time 14864803242.312311 gives back the timestamp
> "2441-01-17 09:00:42.312312+01", even if using rint() (see below).

Hm, that particular case works for me using the patch I committed
yesterday (which just added a rint() call to the existing code).
I'm a bit skeptical of the version you propose here because it seems
mighty prone to subtractive cancellation.  You're basically computing
x - int(x) which can't add any precision that wasn't there before.
Looking at successive microsecond values in this range:

regression=# select 14864803242.312310::float8 - 14864803242;    ?column?      
-------------------0.312309265136719
(1 row)

regression=# select 14864803242.312311::float8 - 14864803242;    ?column?      
-------------------0.312311172485352
(1 row)

regression=# select 14864803242.312312::float8 - 14864803242;    ?column?      
-------------------0.312311172485352
(1 row)

regression=# select 14864803242.312313::float8 - 14864803242;    ?column?      
-------------------0.312313079833984
(1 row)

Basically, 1 ULP in this range is more than 1 microsecond, so there are
going to be places where you cannot get the right answer.  Reformulating
the computation will just shift the errors to nearby values.  float8
simply doesn't have enough bits to represent this many microseconds since
1970 exactly, and the problem's only going to get worse the further out
you look.

I think we might be better advised to add to_timestamp(numeric) alongside
to_timestamp(float8).  There's plenty of precedent for that (e.g, exp(),
ln()) so I would not expect problems with ambiguous function calls.
It'd be slower though ...
        regards, tom lane