Обсуждение: Bad interval conversion?

Hi,
tried on latest 8.5, and some 8.3:
# select '4817191.623 ms'::interval;
     interval
------------------
 -00:35:47.483648
(1 row)

I am pretty sure the answer is wrong. But why?

depesz

--
Linkedin: http://www.linkedin.com/in/depesz  /  blog: http://www.depesz.com/
jid/gtalk: depesz@depesz.com / aim:depeszhdl / skype:depesz_hdl / gg:6749007

On Tue, Aug 18, 2009 at 06:00, hubert depesz
lubaczewski<depesz@depesz.com> wrote:
> Hi,
> tried on latest 8.5, and some 8.3:
> # select '4817191.623 ms'::interval;
>     interval
> ------------------
>  -00:35:47.483648
> (1 row)
>
> I am pretty sure the answer is wrong. But why?

It only happens if you have integer date times on... seems to have
gotten introduced by
http://archives.postgresql.org/pgsql-committers/2008-03/msg00406.php

Im thinking the fact that fsec_t is now an int32 on INT64_TIMESTAMP
builds was a typo/thinko... its really supposed to be int64.

With the attached patch it works correctly and fails with 'out of
range' when its actually out of range.

Alex Hunsaker <badalex@gmail.com> writes:
> It only happens if you have integer date times on... seems to have
> gotten introduced by
> http://archives.postgresql.org/pgsql-committers/2008-03/msg00406.php

Uh, no, fsec_t was int32 before that (look towards the bottom of the
diff).  I'm fairly dubious that fixing it as you suggest is a one-liner
--- the width of fsec_t is something that seems likely to propagate all
over.  A narrower fix for whatever this specific problem is seems safer.

            regards, tom lane

On Tue, Aug 18, 2009 at 10:42, Tom Lane<tgl@sss.pgh.pa.us> wrote:
> Alex Hunsaker <badalex@gmail.com> writes:
>> It only happens if you have integer date times on... seems to have
>> gotten introduced by
>> http://archives.postgresql.org/pgsql-committers/2008-03/msg00406.php
>
> Uh, no, fsec_t was int32 before that (look towards the bottom of the
> diff).  I'm fairly dubious that fixing it as you suggest is a one-liner
> --- the width of fsec_t is something that seems likely to propagate all
> over.  A narrower fix for whatever this specific problem is seems safer.

(not to mention it probably breaks ecpg ...)

I saw:
typedef int32 fsec_t;
vs
typedef double fsec_t;

and thought hrm... that looks odd..

Ok well we can add overflow checks where we need-em.  If you don't
think the attached patch is horridly ugly- im willing wade through the
uses of fsec and apply something similar where we need them.
(DTK_SECOND at the very least, but fsec_t stuff is scattered all
through adt/)

*** a/src/backend/utils/adt/datetime.c
--- b/src/backend/utils/adt/datetime.c
***************
*** 2987,2993 **** DecodeInterval(char **field, int *ftype, int nf, int range,

                      case DTK_MILLISEC:
  #ifdef HAVE_INT64_TIMESTAMP
!                         *fsec += rint((val + fval) * 1000);
  #else
                          *fsec += (val + fval) * 1e-3;
  #endif
--- 2987,3001 ----

                      case DTK_MILLISEC:
  #ifdef HAVE_INT64_TIMESTAMP
!                         /*
!                          * fval is unused or re-initialized if it is
!                          * needed again */
!                         fval = rint((val + fval) * 1000);
!
!                         if (fval < INT_MIN || fval > INT_MAX)
!                             return DTERR_FIELD_OVERFLOW;
!
!                         *fsec += fval;
  #else
                          *fsec += (val + fval) * 1e-3;
  #endif

Alex Hunsaker <badalex@gmail.com> writes:
> Ok well we can add overflow checks where we need-em.  If you don't
> think the attached patch is horridly ugly- im willing wade through the
> uses of fsec and apply something similar where we need them.

Throwing overflow errors doesn't seem very nice either, especially not
for values that worked just fine before 8.4.

I think the reason fsec_t is declared int32 is that it's expected to
hold only *fractional* seconds, ie, not more than 1 million in the
integer-timestamp world.  The code snippets you point at are violating
that assumption.  Which is something the float code paths could get away
with, but not so much for integer timestamps.  Even if we widened the
type to int64, it'd still mean that the overflow threshold is a factor
of 1e6 closer in these code paths than in other places, and thus that
there would still be values that failed in integer timestamp arithmetic
but not float timestamp arithmetic.

Seems like a proper fix would involve doing some modulo arithmetic to be
sure that we add the integral seconds to the seconds field and only a
fraction to the fsec field.

            regards, tom lane

On Tue, Aug 18, 2009 at 12:07, Tom Lane<tgl@sss.pgh.pa.us> wrote:
> Throwing overflow errors doesn't seem very nice either, especially not
> for values that worked just fine before 8.4.

I just checked both 8.3.7 and 8.2.13 give:
# select '4817191.623 ms'::interval;
    interval
------------------
 -00:35:47.483648
(1 row)


> Seems like a proper fix would involve doing some modulo arithmetic to be
> sure that we add the integral seconds to the seconds field and only a
> fraction to the fsec field.

Ok I looked around at the other fsec assignments in adt/ and did not
see any that were not treating them as fractional correctly.  This
seems to be the only case.  Anywho is the below more what you
expected? (I decided to do it for the floating point case as well...)

With this patch I get (it also passes a make check):
# select '4817191.623 ms'::interval;
    interval
-----------------
 01:20:17.191623


*** a/src/backend/utils/adt/datetime.c
--- b/src/backend/utils/adt/datetime.c
***************
*** 2986,2991 **** DecodeInterval(char **field, int *ftype, int nf, int range,
--- 2986,2994 ----
                                                break;

                                        case DTK_MILLISEC:
+                                               tm->tm_sec += val / 1000;
+                                               val = val % 1000;
+
  #ifdef HAVE_INT64_TIMESTAMP
                                                *fsec += rint((val +
fval) * 1000);
  #else

Alex Hunsaker <badalex@gmail.com> writes:
> On Tue, Aug 18, 2009 at 12:07, Tom Lane<tgl@sss.pgh.pa.us> wrote:
>> Throwing overflow errors doesn't seem very nice either, especially not
>> for values that worked just fine before 8.4.

> I just checked both 8.3.7 and 8.2.13 give:
> # select '4817191.623 ms'::interval;
>     interval
> ------------------
>  -00:35:47.483648
> (1 row)

Presumably you mean "8.3.7 and 8.2.13 built with the nonstandard
--enable-integer-datetimes option".  The reason I find this a big issue
is that the default behavior has changed as of 8.4.  We'll want to
back-patch of course, but the point is that these cases do work as
expected under float timestamps.

> Ok I looked around at the other fsec assignments in adt/ and did not
> see any that were not treating them as fractional correctly.  This
> seems to be the only case.  Anywho is the below more what you
> expected? (I decided to do it for the floating point case as well...)

I'll take a closer look later, but it seems reasonably sane at first
glance.  Thanks for doing the legwork!

            regards, tom lane

> Ok I looked around at the other fsec assignments in adt/ and did not
> see any that were not treating them as fractional correctly.  This
> seems to be the only case.  Anywho is the below more what you
> expected? (I decided to do it for the floating point case as well...)

Applied with some cosmetic adjustments.  (Some of us old hands still
don't trust / and % to be consistent for negative inputs ...)

            regards, tom lane