Re: [HACKERS] Interval aggregate regression failure (expected - Mailing list pgsql-patches

From Bruce Momjian
Subject Re: [HACKERS] Interval aggregate regression failure (expected
Date
Msg-id 200608260240.k7Q2eL124053@momjian.us
Whole thread Raw
Responses Re: [HACKERS] Interval aggregate regression failure (expected seems  (Michael Glaesemann <grzm@seespotcode.net>)
List pgsql-patches
I used your ideas to make a patch to fix your example:

    test=> select '41 months'::interval  / 10;
       ?column?
    ---------------
     4 mons 3 days
    (1 row)

and

    test=> select '41 months'::interval  * 0.3;
       ?column?
    ---------------
     1 year 9 days
    (1 row)

The trick was not to play with the division, but to check if the number
of seconds cascaded into full days and/or months.

---------------------------------------------------------------------------

Tom Lane wrote:
> Michael Glaesemann <grzm@seespotcode.net> writes:
> > ... I think this just confirms that there is some kind of rounding (or
> > lack of) in interval_div. Kind of frustrating that it's not visible
> > in the result.
>
> I think the fundamental problem is that the float8 results of division
> are inaccurate, and yet we're assuming that we can (for instance) coerce
> them to integer and get exactly the right answer.  For instance, in the
> '41 months'/10 example, I get month_remainder_days being computed as
>
> (gdb) p month_remainder
> $19 = 0.099999999999999645
> (gdb) s
> 2575            result->day += (int32) month_remainder_days;
> (gdb) p month_remainder_days
> $20 = 2.9999999999999893
>
> The only way we can really fix this is to be willing to round off
> the numbers, and I think the only principled way to do that is to
> settle on a specific target accuracy, probably 1 microsecond.
> Then the thing to do would be to scale up all the intermediate
> float results to microseconds and apply rint().  Something like
> (untested)
>
>     month_remainder = rint(span->month * USECS_PER_MONTH / factor);
>     day_remainder = rint(span->day * USECS_PER_DAY / factor);
>     result->month = (int32) (month_remainder / USECS_PER_MONTH);
>     result->day = (int32) (day_remainder / USECS_PER_DAY);
>     month_remainder -= result->month * USECS_PER_MONTH;
>     day_remainder -= result->day * USECS_PER_DAY;
>
>     /*
>      * Handle any fractional parts the same way as in interval_mul.
>      */
>
>     /* fractional months full days into days */
>     month_remainder_days = month_remainder * DAYS_PER_MONTH;
>     extra_days = (int32) (month_remainder_days / USECS_PER_DAY);
>     result->day += extra_days;
>     /* fractional months partial days into time */
>     day_remainder += month_remainder_days - extra_days * USECS_PER_DAY;
>
> #ifdef HAVE_INT64_TIMESTAMP
>     result->time = rint(span->time / factor + day_remainder);
> #else
>     result->time = rint(span->time * 1.0e6 / factor + day_remainder) / 1.0e6;
> #endif
>
> This might need a few more rint() calls --- I'm assuming that float ops
> with exact integral inputs will be OK, which is an assumption used
> pretty widely in the datetime code, but ...
>
> Per the comment, if we do this here we probably want to make
> interval_mul work similarly.
>
>             regards, tom lane

--
  Bruce Momjian   bruce@momjian.us
  EnterpriseDB    http://www.enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +
Index: src/backend/utils/adt/timestamp.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/adt/timestamp.c,v
retrieving revision 1.165
diff -c -c -r1.165 timestamp.c
*** src/backend/utils/adt/timestamp.c    13 Jul 2006 16:49:16 -0000    1.165
--- src/backend/utils/adt/timestamp.c    26 Aug 2006 02:36:28 -0000
***************
*** 2526,2531 ****
--- 2526,2546 ----
      result->time = rint(span->time * factor + day_remainder * USECS_PER_DAY);
  #else
      result->time = span->time * factor + day_remainder * SECS_PER_DAY;
+     /*
+      *    The imprecision of float8 causes unusual rounding of even integer
+      *    division, so round up if we have full units.  Check seconds only
+      *    as far as microseconds.
+      */
+     if (TSROUND(result->time) == SECS_PER_DAY)
+     {
+         result->day++;
+         result->time = 0;
+     }
+     if (result->day == DAYS_PER_MONTH)
+     {
+         result->month++;
+         result->day = 0;
+     }
  #endif

      PG_RETURN_INTERVAL_P(result);
***************
*** 2579,2584 ****
--- 2594,2614 ----
      result->time = rint(span->time / factor + day_remainder * USECS_PER_DAY);
  #else
      result->time = span->time / factor + day_remainder * SECS_PER_DAY;
+     /*
+      *    The imprecision of float8 causes unusual rounding of even integer
+      *    division, so round up if we have full units.  Check seconds only
+      *    as far as microseconds.
+      */
+     if (TSROUND(result->time) == SECS_PER_DAY)
+     {
+         result->day++;
+         result->time = 0;
+     }
+     if (result->day == DAYS_PER_MONTH)
+     {
+         result->month++;
+         result->day = 0;
+     }
  #endif

      PG_RETURN_INTERVAL_P(result);

pgsql-patches by date:

Previous
From: Bruce Momjian
Date:
Subject: Re: plpython improvements
Next
From: Bruce Momjian
Date:
Subject: Re: Adding fulldisjunctions to the contrib