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

Bruce Momjian wrote:
> Michael Glaesemann wrote:
> >
> > On Sep 3, 2006, at 12:34 , Bruce Momjian wrote:
> >
> > > OK, I worked with Michael and I think this is the best we are going to
> > > do to fix this.  It has one TSROUND call for Powerpc, and that is
> > > documented.  Applied.
> >
> > As I was working up regression tests, I found a case that this patch
> > doesn't handle.
> >
> > select interval '4 mon' * .3 as product_h;
> >         product_h
> > -----------------------
> > 1 mon 5 days 24:00:00
> > (1 row)
> >
> > This should be 1 mon 6 days. It fails for any number of months
> > greater than 3 that is not evenly divisible by 10, greater than 3
> > months. Do we need to look at the month remainder separately?
>
> Another question.  Is this result correct?
>
>     test=> select '999 months 999 days'::interval / 100;
>             ?column?
>     -------------------------
>      9 mons 38 days 40:33:36
>     (1 row)
>
> Should that be:
>
>      9 mons 39 days 16:33:36
>
> The core problem is that the combined remainder seconds of months and
> days is > 24 hours.

OK, updated patch.  It will fix the >=24:00:00 case because it cascades
up if the remainder number of seconds is greater or equal to one day.
One open item is that it still might show >24 hours if the seconds
computation combined with the remaning seconds >24 hours.  Not sure if
that should be handled or not.  If you fix that, you really are
cascading up because the resulting seconds might be less than the
computed value, e.g. result is 23:00:00, remainder is 02:00:00, cascade
up would be 1 day, 01:00:00.  I am unsure we want to do that.  Right
now, this will show 25:00:00.

--
  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.166
diff -c -c -r1.166 timestamp.c
*** src/backend/utils/adt/timestamp.c    3 Sep 2006 03:34:04 -0000    1.166
--- src/backend/utils/adt/timestamp.c    4 Sep 2006 03:49:21 -0000
***************
*** 2525,2541 ****
      sec_remainder = (orig_day * (double)SECS_PER_DAY) * factor -
              result->day * (double)SECS_PER_DAY +
              (month_remainder_days - (int32) month_remainder_days) * SECS_PER_DAY;

      /* cascade units down */
      result->day += (int32) month_remainder_days;
  #ifdef HAVE_INT64_TIMESTAMP
      result->time = rint(span->time * factor + sec_remainder * USECS_PER_SEC);
  #else
!     /*
!      *    TSROUND() needed to prevent -146:23:60.00 output on PowerPC for
!      *    SELECT interval '-41 mon -12 days -360:00' * 0.3;
!      */
!     result->time = span->time * factor + TSROUND(sec_remainder);
  #endif

      PG_RETURN_INTERVAL_P(result);
--- 2524,2547 ----
      sec_remainder = (orig_day * (double)SECS_PER_DAY) * factor -
              result->day * (double)SECS_PER_DAY +
              (month_remainder_days - (int32) month_remainder_days) * SECS_PER_DAY;
+     /*
+      *    Might have 24:00:00 hours due to rounding, or >24 hours because of
+      *    time cascade from months and days.  It might still be >24 if the
+      *    combination of cascade and the seconds factor operation itself.
+      */
+     sec_remainder = TSROUND(sec_remainder);
+     if (Abs(sec_remainder) >= SECS_PER_DAY)
+     {
+         sec_remainder -= (int)(sec_remainder / SECS_PER_DAY) * SECS_PER_DAY;
+         result->day += (int)(sec_remainder / SECS_PER_DAY);
+     }

      /* cascade units down */
      result->day += (int32) month_remainder_days;
  #ifdef HAVE_INT64_TIMESTAMP
      result->time = rint(span->time * factor + sec_remainder * USECS_PER_SEC);
  #else
!     result->time = span->time * factor + sec_remainder;
  #endif

      PG_RETURN_INTERVAL_P(result);
***************
*** 2579,2584 ****
--- 2585,2596 ----
      sec_remainder = (orig_day * (double)SECS_PER_DAY) / factor -
              result->day * (double)SECS_PER_DAY +
              (month_remainder_days - (int32) month_remainder_days) * SECS_PER_DAY;
+     sec_remainder = TSROUND(sec_remainder);
+     if (Abs(sec_remainder) >= SECS_PER_DAY)
+     {
+         sec_remainder -= (int)(sec_remainder / SECS_PER_DAY) * SECS_PER_DAY;
+         result->day += (int)(sec_remainder / SECS_PER_DAY);
+     }

      /* cascade units down */
      result->day += (int32) month_remainder_days;
***************
*** 2586,2592 ****
      result->time = rint(span->time / factor + sec_remainder * USECS_PER_SEC);
  #else
      /* See TSROUND comment in interval_mul(). */
!     result->time = span->time / factor + TSROUND(sec_remainder);
  #endif

      PG_RETURN_INTERVAL_P(result);
--- 2598,2604 ----
      result->time = rint(span->time / factor + sec_remainder * USECS_PER_SEC);
  #else
      /* See TSROUND comment in interval_mul(). */
!     result->time = span->time / factor + sec_remainder;
  #endif

      PG_RETURN_INTERVAL_P(result);
Index: src/include/utils/timestamp.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/utils/timestamp.h,v
retrieving revision 1.62
diff -c -c -r1.62 timestamp.h
*** src/include/utils/timestamp.h    13 Jul 2006 16:49:20 -0000    1.62
--- src/include/utils/timestamp.h    4 Sep 2006 03:49:22 -0000
***************
*** 161,171 ****

  typedef double fsec_t;

! /* round off to MAX_TIMESTAMP_PRECISION decimal places */
! /* note: this is also used for rounding off intervals */
  #define TS_PREC_INV 1000000.0
  #define TSROUND(j) (rint(((double) (j)) * TS_PREC_INV) / TS_PREC_INV)
- #endif

  #define TIMESTAMP_MASK(b) (1 << (b))
  #define INTERVAL_MASK(b) (1 << (b))
--- 161,174 ----

  typedef double fsec_t;

! #endif
!
! /*
!  *    Round off to MAX_TIMESTAMP_PRECISION decimal places.
!  *    Note: this is also used for rounding off intervals.
!  */
  #define TS_PREC_INV 1000000.0
  #define TSROUND(j) (rint(((double) (j)) * TS_PREC_INV) / TS_PREC_INV)

  #define TIMESTAMP_MASK(b) (1 << (b))
  #define INTERVAL_MASK(b) (1 << (b))