Обсуждение: Re: [COMMITTERS] pgsql: Allow to_timestamp(float8) to convert float infinity to timestam

On Wed, Mar 30, 2016 at 6:09 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Allow to_timestamp(float8) to convert float infinity to timestamp infinity.
>
> With the original SQL-function implementation, such cases failed because
> we don't support infinite intervals.  Converting the function to C lets
> us bypass the interval representation, which should be a bit faster as
> well as more flexible.

+-- The upper boundary differs between integer and float timestamps,
so check the biggest one
+SELECT to_timestamp(185331707078400::float8);  -- error, out of range
+ERROR:  timestamp out of range: "1.85332e+14"
Some of the tests introduced are making MSVC unhappy, because they
depend on the three-digit behavior that Windows is using, leading to
those failures:
 -- The upper boundary differs between integer and float timestamps,
so check the biggest one SELECT to_timestamp(185331707078400::float8);  -- error, out of range
! ERROR:  timestamp out of range: "1.85332e+14" -- nonfinite values SELECT to_timestamp(' Infinity'::float);
to_timestamp
--- 2338,2344 ----
 -- The upper boundary differs between integer and float timestamps,
so check the biggest one SELECT to_timestamp(185331707078400::float8);  -- error, out of range
! ERROR:  timestamp out of range: "1.85332e+014"

If the those tests are kept, an alternate output file is necessary (I
can send a patch if needed, I see the failure locally as well).
-- 
Michael

Michael Paquier <michael.paquier@gmail.com> writes:
> On Wed, Mar 30, 2016 at 6:09 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Allow to_timestamp(float8) to convert float infinity to timestamp infinity.

> Some of the tests introduced are making MSVC unhappy, because they
> depend on the three-digit behavior that Windows is using, leading to
> those failures:

Ah, I was wondering about that.  The patch as-submitted used "%lf" which
seemed even less likely to be portable, but evidently %g isn't that much
better.

> If the those tests are kept, an alternate output file is necessary (I
> can send a patch if needed, I see the failure locally as well).

I'm inclined to just drop the out-of-range test cases.  They're not that
useful IMO, and alternate expected-files are a real PITA for maintenance.
        regards, tom lane

On Wed, Mar 30, 2016 at 10:13 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Michael Paquier <michael.paquier@gmail.com> writes:
>> On Wed, Mar 30, 2016 at 6:09 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Allow to_timestamp(float8) to convert float infinity to timestamp infinity.
>
>> Some of the tests introduced are making MSVC unhappy, because they
>> depend on the three-digit behavior that Windows is using, leading to
>> those failures:
>
> Ah, I was wondering about that.  The patch as-submitted used "%lf" which
> seemed even less likely to be portable, but evidently %g isn't that much
> better.

Yep.

>> If the those tests are kept, an alternate output file is necessary (I
>> can send a patch if needed, I see the failure locally as well).
>
> I'm inclined to just drop the out-of-range test cases.  They're not that
> useful IMO, and alternate expected-files are a real PITA for maintenance.

Hm. Actually, they are quite useful to check error boundaries, so why
not just simplifying the error message to "timestamp out of range" and
remove the value from it?
-- 
Michael

Michael Paquier <michael.paquier@gmail.com> writes:
> On Wed, Mar 30, 2016 at 10:13 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I'm inclined to just drop the out-of-range test cases.  They're not that
>> useful IMO, and alternate expected-files are a real PITA for maintenance.

> Hm. Actually, they are quite useful to check error boundaries, so why
> not just simplifying the error message to "timestamp out of range" and
> remove the value from it?

Meh.  I realize that there are a lot of places where we just say
"timestamp out of range" rather than trying to give a specific value,
but it's really contrary to our message style guidelines to not print
the complained-of value.  I think we should leave the ereport calls as-is
and remove the test cases; to do otherwise is putting the regression tests
ahead of users.  The upper-boundary test is quite dubiously useful anyway,
because it has to test a value that's very far away from where the
boundary actually is in most builds.
        regards, tom lane

On Wed, Mar 30, 2016 at 10:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Michael Paquier <michael.paquier@gmail.com> writes:
>> On Wed, Mar 30, 2016 at 10:13 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> I'm inclined to just drop the out-of-range test cases.  They're not that
>>> useful IMO, and alternate expected-files are a real PITA for maintenance.
>
>> Hm. Actually, they are quite useful to check error boundaries, so why
>> not just simplifying the error message to "timestamp out of range" and
>> remove the value from it?
>
> Meh.  I realize that there are a lot of places where we just say
> "timestamp out of range" rather than trying to give a specific value,
> but it's really contrary to our message style guidelines to not print
> the complained-of value.  I think we should leave the ereport calls as-is
> and remove the test cases; to do otherwise is putting the regression tests
> ahead of users.  The upper-boundary test is quite dubiously useful anyway,
> because it has to test a value that's very far away from where the
> boundary actually is in most builds.

OK, I won't fight more on that.
-- 
Michael