Thread: 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.
Attachment
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
Attachment
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
Attachment
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