Обсуждение: regression failure on master with --disable-integer-datetimes

Introduced in commit 4318daecc959886d001a6e79c6ea853e8b1dfb4b.

Attached regression failure and patch. It looks like we're no longer
running floating point timestamps on the buildfarm?

To make the output for float/integer timestamps the same, we either need
to remove the fractional seconds or reduce the number of hours.

It looks like the test is for maximum output length of an interval, so
it seems better to reduce the number of hours to one million, which is
what my patch does. It still loses 3 characters of output length, which
is a bit unfortunate given the purpose of the test, but it's better than
losing 7. I don't have a better idea.

Regards,
    Jeff Davis

Jeff Davis <pgsql@j-davis.com> writes:
> Introduced in commit 4318daecc959886d001a6e79c6ea853e8b1dfb4b.
> Attached regression failure and patch. It looks like we're no longer
> running floating point timestamps on the buildfarm?

Evidently :-(.  Who wants to crank up an animal doing that?

> To make the output for float/integer timestamps the same, we either need
> to remove the fractional seconds or reduce the number of hours.

> It looks like the test is for maximum output length of an interval, so
> it seems better to reduce the number of hours to one million, which is
> what my patch does. It still loses 3 characters of output length, which
> is a bit unfortunate given the purpose of the test, but it's better than
> losing 7. I don't have a better idea.

Hm.  As the test stands, it requires a float-timestamps implementation
to store a value of 3600000610.000001 seconds, which is 16 decimal digits
or about one more than can reliably be extracted from an IEEE float8.
At least for IEEE machines, I think we could remove just one digit
and it would work (or if not, that would say that some additional thought
needs to be put into the rounding behavior in interval_out).

However, removing *any* digits seems like it mostly defeats the point of
the test.  Maybe we should just lose the test?

A different solution is to add a variant expected-output file, though
I'm not terribly thrilled with that answer.

            regards, tom lane

On Tue, 2014-05-06 at 14:48 -0400, Tom Lane wrote:
> Hm.  As the test stands, it requires a float-timestamps implementation
> to store a value of 3600000610.000001 seconds, which is 16 decimal digits

The test value has 1e9 hours, which is 3.6e12 seconds, plus 6 more
digits for microseconds is 18 decimal digits. If, as you say, 15 digits
can be reliably extracted from a double, then we need to cut three zeros
(which matches my simple test of just removing zeros until it achieves
the microsecond precision).

> However, removing *any* digits seems like it mostly defeats the point of
> the test.  Maybe we should just lose the test?

The reason I thought it might still have some value is because it would
have still caught the problem that 4318daec patched. But I'm fine with
removing it.

(Though, I should add a comment indicating that it's not testing the
true maximum length.)

> A different solution is to add a variant expected-output file, though
> I'm not terribly thrilled with that answer.

Nor am I.

Regards,
    Jeff Davis

Jeff Davis <pgsql@j-davis.com> writes:
> On Tue, 2014-05-06 at 14:48 -0400, Tom Lane wrote:
>> Hm.  As the test stands, it requires a float-timestamps implementation
>> to store a value of 3600000610.000001 seconds, which is 16 decimal digits

> The test value has 1e9 hours, which is 3.6e12 seconds, plus 6 more
> digits for microseconds is 18 decimal digits. If, as you say, 15 digits
> can be reliably extracted from a double, then we need to cut three zeros
> (which matches my simple test of just removing zeros until it achieves
> the microsecond precision).

Um.  I fat-fingered the math somehow ... I think I might've
copied-and-pasted from your modified test rather than the original.
But the bad news there is that you have too: 3.6e12 has got 13
digits to the left of the decimal point, leaving only 2 that could
be reliably extracted to the right.  The correct math, if I've not
messed up again, is that this is the seconds value that the test
is trying to store:

regression=# select 1000000000.0*3600 + 10*60 + 10.000001;
       ?column?
----------------------
 3600000000610.000001
(1 row)

which cannot be reproduced by float8:

regression=# select '3600000000610.000001'::float8;
    float8
---------------
 3600000000610
(1 row)

If we trim digits to the left of the decimal point, we need to
get rid of 4 not 3:

regression=# select '360000000610.000001'::float8;
    float8
--------------
 360000000610
(1 row)

regression=# select '36000000610.000001'::float8;
   float8
-------------
 36000000610
(1 row)

regression=# select '3600000610.000001'::float8;
   float8
------------
 3600000610
(1 row)

regression=# select '360000610.000001'::float8;
      float8
------------------
 360000610.000001
(1 row)

If we instead trim digits at the right, we have to go down to just 2,
at least on my Linux/x86_64 platform:

regression=# select '3600000000610.001'::float8;
    float8
---------------
 3600000000610
(1 row)

regression=# select '3600000000610.01'::float8;
      float8
------------------
 3600000000610.01
(1 row)

which squares with the expectation that you get between 15 and 16
decimal digits of precision from a float8.

In short, your revised test might work for you but I think we'd
need to drop another zero to have much confidence of it working
everywhere.

            regards, tom lane

On Tue, May 06, 2014 at 12:18:02PM -0700, Jeff Davis wrote:
> On Tue, 2014-05-06 at 14:48 -0400, Tom Lane wrote:
> > However, removing *any* digits seems like it mostly defeats the point of
> > the test.  Maybe we should just lose the test?
>
> The reason I thought it might still have some value is because it would
> have still caught the problem that 4318daec patched. But I'm fine with
> removing it.
>
> (Though, I should add a comment indicating that it's not testing the
> true maximum length.)

I'm good with how you fixed it; losing three digits wouldn't be grave.  I
might fix it like this instead:

--- a/src/test/regress/sql/interval.sql
+++ b/src/test/regress/sql/interval.sql
@@ -111,3 +111,5 @@ select '4 millenniums 5 centuries 4 decades 1 year 4 months 4 days 17 minutes 31
 -- test long interval output
-select '100000000y 10mon -1000000000d -1000000000h -10min -10.000001s ago'::interval;
+select '100000000y 10mon -1000000000d -1000000000h -10min -10.000001s ago'::interval::text IN
+('@ 100000000 years 10 mons -1000000000 days -1000000000 hours -10 mins -10.000001 secs ago',
+ '@ 100000000 years 10 mons -1000000000 days -1000000000 hours -10 mins -10 secs ago');

It seems I also broke the ecpg test suite under --disable-integer-datetimes.
I think (not tested) the fix is s/123456789/123456123/ in dt_test2.pgc,
because it rounds under float datetimes and truncates under integer datetimes.

> > A different solution is to add a variant expected-output file, though
> > I'm not terribly thrilled with that answer.
>
> Nor am I.

Agreed.

--
Noah Misch
EnterpriseDB                                 http://www.enterprisedb.com

On Tue, May 06, 2014 at 03:39:52PM -0400, Noah Misch wrote:
> It seems I also broke the ecpg test suite under --disable-integer-datetimes.
> I think (not tested) the fix is s/123456789/123456123/ in dt_test2.pgc,
> because it rounds under float datetimes and truncates under integer datetimes.

Besides that, one of the new combinations had the same insufficient-precision
problem that affected interval.sql.  With your commit and the attached patch,
"make check-world" looks good under --disable-integer-datetimes.

--
Noah Misch
EnterpriseDB                                 http://www.enterprisedb.com