Patch: AdjustIntervalForTypmod shouldn't discard high-order data - Mailing list pgsql-hackers

As I mentioned a bit ago
http://archives.postgresql.org/pgsql-hackers/2009-05/msg01505.php
there seems to be a definite problem still remaining with our handling
of interval literals.  To wit, this behavior is absolutely not per spec:

regression=# select '999'::interval second;
 interval
----------
 00:00:39
(1 row)

The correct interpretation of the input value is certainly 999 seconds.
The spec would allow us to throw error if it exceeds the available range
of the field, but a silent modulo operation is not per spec and seems
against our general design principle of not silently discarding data.
I propose the attached patch to make the code not throw away high-order
values in this fashion.

A somewhat more debatable case is this:

regression=# select '1 day 1 hour'::interval hour;
 interval
----------
 01:00:00
(1 row)

which with the attached patch we would render as

regression=# select '1 day 1 hour'::interval hour;
    interval
----------------
 1 day 01:00:00
(1 row)

There is some case to be made that we should throw error here,
which we could do by putting error tests where the attached patch
has comments suggesting an error test.  However I'm inclined to think
that doing that would expose an implementation dependency rather more
than we should.  It is usually not clear to novices that '1 day 1 hour'
is different from '25 hours', and it would be even less clear why the
latter would be acceptable input for an INTERVAL HOUR field when the
former isn't.  So I'm proposing the patch as-is rather than with the
extra error tests, but am open to being convinced otherwise.

The reason I'm bringing this up now is that we've already changed the
behavior of interval literals quite a bit in 8.4.  I would rather try to
finish getting it right in this release than have the behavior change
twice in successive releases.

Comments?

            regards, tom lane

Index: src/backend/utils/adt/timestamp.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/adt/timestamp.c,v
retrieving revision 1.199
diff -c -r1.199 timestamp.c
*** src/backend/utils/adt/timestamp.c    26 May 2009 02:17:50 -0000    1.199
--- src/backend/utils/adt/timestamp.c    31 May 2009 22:13:35 -0000
***************
*** 962,967 ****
--- 962,979 ----
          int            range = INTERVAL_RANGE(typmod);
          int            precision = INTERVAL_PRECISION(typmod);

+         /*
+          * Our interpretation of intervals with a limited set of fields
+          * is that fields to the right of the last one specified are zeroed
+          * out, but those to the left of it remain valid.  Since we do not
+          * have any equivalent of SQL's <interval leading field precision>,
+          * we can't properly enforce any limit on the leading field.  (Before
+          * PG 8.4 we interpreted a limited set of fields as actually causing
+          * a "modulo" operation on a given value, potentially losing high-order
+          * as well as low-order information; but there is no support for such
+          * behavior in the standard.)
+          */
+
          if (range == INTERVAL_FULL_RANGE)
          {
              /* Do nothing... */
***************
*** 974,1000 ****
          }
          else if (range == INTERVAL_MASK(MONTH))
          {
-             interval->month %= MONTHS_PER_YEAR;
              interval->day = 0;
              interval->time = 0;
          }
          /* YEAR TO MONTH */
          else if (range == (INTERVAL_MASK(YEAR) | INTERVAL_MASK(MONTH)))
          {
-             /* month is already year to month */
              interval->day = 0;
              interval->time = 0;
          }
          else if (range == INTERVAL_MASK(DAY))
          {
!             interval->month = 0;
              interval->time = 0;
          }
          else if (range == INTERVAL_MASK(HOUR))
          {
!             interval->month = 0;
!             interval->day = 0;
!
  #ifdef HAVE_INT64_TIMESTAMP
              interval->time = (interval->time / USECS_PER_HOUR) *
                  USECS_PER_HOUR;
--- 986,1008 ----
          }
          else if (range == INTERVAL_MASK(MONTH))
          {
              interval->day = 0;
              interval->time = 0;
          }
          /* YEAR TO MONTH */
          else if (range == (INTERVAL_MASK(YEAR) | INTERVAL_MASK(MONTH)))
          {
              interval->day = 0;
              interval->time = 0;
          }
          else if (range == INTERVAL_MASK(DAY))
          {
!             /* throw error if month is not 0? */
              interval->time = 0;
          }
          else if (range == INTERVAL_MASK(HOUR))
          {
!             /* throw error if month or day is not 0? */
  #ifdef HAVE_INT64_TIMESTAMP
              interval->time = (interval->time / USECS_PER_HOUR) *
                  USECS_PER_HOUR;
***************
*** 1004,1045 ****
          }
          else if (range == INTERVAL_MASK(MINUTE))
          {
!             TimeOffset    hour;
!
!             interval->month = 0;
!             interval->day = 0;
!
  #ifdef HAVE_INT64_TIMESTAMP
-             hour = interval->time / USECS_PER_HOUR;
-             interval->time -= hour * USECS_PER_HOUR;
              interval->time = (interval->time / USECS_PER_MINUTE) *
                  USECS_PER_MINUTE;
  #else
-             TMODULO(interval->time, hour, (double) SECS_PER_HOUR);
              interval->time = ((int) (interval->time / SECS_PER_MINUTE)) * (double) SECS_PER_MINUTE;
  #endif
          }
          else if (range == INTERVAL_MASK(SECOND))
          {
!             TimeOffset    minute;
!
!             interval->month = 0;
!             interval->day = 0;
!
! #ifdef HAVE_INT64_TIMESTAMP
!             minute = interval->time / USECS_PER_MINUTE;
!             interval->time -= minute * USECS_PER_MINUTE;
! #else
!             TMODULO(interval->time, minute, (double) SECS_PER_MINUTE);
!             /* return subseconds too */
! #endif
          }
          /* DAY TO HOUR */
          else if (range == (INTERVAL_MASK(DAY) |
                             INTERVAL_MASK(HOUR)))
          {
!             interval->month = 0;
!
  #ifdef HAVE_INT64_TIMESTAMP
              interval->time = (interval->time / USECS_PER_HOUR) *
                  USECS_PER_HOUR;
--- 1012,1035 ----
          }
          else if (range == INTERVAL_MASK(MINUTE))
          {
!             /* throw error if month or day is not 0? */
  #ifdef HAVE_INT64_TIMESTAMP
              interval->time = (interval->time / USECS_PER_MINUTE) *
                  USECS_PER_MINUTE;
  #else
              interval->time = ((int) (interval->time / SECS_PER_MINUTE)) * (double) SECS_PER_MINUTE;
  #endif
          }
          else if (range == INTERVAL_MASK(SECOND))
          {
!             /* throw error if month or day is not 0? */
!             /* for the moment, leave fractional seconds as-is */
          }
          /* DAY TO HOUR */
          else if (range == (INTERVAL_MASK(DAY) |
                             INTERVAL_MASK(HOUR)))
          {
!             /* throw error if month is not 0? */
  #ifdef HAVE_INT64_TIMESTAMP
              interval->time = (interval->time / USECS_PER_HOUR) *
                  USECS_PER_HOUR;
***************
*** 1052,1059 ****
                             INTERVAL_MASK(HOUR) |
                             INTERVAL_MASK(MINUTE)))
          {
!             interval->month = 0;
!
  #ifdef HAVE_INT64_TIMESTAMP
              interval->time = (interval->time / USECS_PER_MINUTE) *
                  USECS_PER_MINUTE;
--- 1042,1048 ----
                             INTERVAL_MASK(HOUR) |
                             INTERVAL_MASK(MINUTE)))
          {
!             /* throw error if month is not 0? */
  #ifdef HAVE_INT64_TIMESTAMP
              interval->time = (interval->time / USECS_PER_MINUTE) *
                  USECS_PER_MINUTE;
***************
*** 1066,1080 ****
                             INTERVAL_MASK(HOUR) |
                             INTERVAL_MASK(MINUTE) |
                             INTERVAL_MASK(SECOND)))
!             interval->month = 0;
!
          /* HOUR TO MINUTE */
          else if (range == (INTERVAL_MASK(HOUR) |
                             INTERVAL_MASK(MINUTE)))
          {
!             interval->month = 0;
!             interval->day = 0;
!
  #ifdef HAVE_INT64_TIMESTAMP
              interval->time = (interval->time / USECS_PER_MINUTE) *
                  USECS_PER_MINUTE;
--- 1055,1069 ----
                             INTERVAL_MASK(HOUR) |
                             INTERVAL_MASK(MINUTE) |
                             INTERVAL_MASK(SECOND)))
!         {
!             /* throw error if month is not 0? */
!             /* for the moment, leave fractional seconds as-is */
!         }
          /* HOUR TO MINUTE */
          else if (range == (INTERVAL_MASK(HOUR) |
                             INTERVAL_MASK(MINUTE)))
          {
!             /* throw error if month or day is not 0? */
  #ifdef HAVE_INT64_TIMESTAMP
              interval->time = (interval->time / USECS_PER_MINUTE) *
                  USECS_PER_MINUTE;
***************
*** 1087,1111 ****
                             INTERVAL_MASK(MINUTE) |
                             INTERVAL_MASK(SECOND)))
          {
!             interval->month = 0;
!             interval->day = 0;
!             /* return subseconds too */
          }
          /* MINUTE TO SECOND */
          else if (range == (INTERVAL_MASK(MINUTE) |
                             INTERVAL_MASK(SECOND)))
          {
!             TimeOffset    hour;
!
!             interval->month = 0;
!             interval->day = 0;
!
! #ifdef HAVE_INT64_TIMESTAMP
!             hour = interval->time / USECS_PER_HOUR;
!             interval->time -= hour * USECS_PER_HOUR;
! #else
!             TMODULO(interval->time, hour, (double) SECS_PER_HOUR);
! #endif
          }
          else
              elog(ERROR, "unrecognized interval typmod: %d", typmod);
--- 1076,1090 ----
                             INTERVAL_MASK(MINUTE) |
                             INTERVAL_MASK(SECOND)))
          {
!             /* throw error if month or day is not 0? */
!             /* for the moment, leave fractional seconds as-is */
          }
          /* MINUTE TO SECOND */
          else if (range == (INTERVAL_MASK(MINUTE) |
                             INTERVAL_MASK(SECOND)))
          {
!             /* throw error if month or day is not 0? */
!             /* for the moment, leave fractional seconds as-is */
          }
          else
              elog(ERROR, "unrecognized interval typmod: %d", typmod);
Index: src/test/regress/expected/interval.out
===================================================================
RCS file: /cvsroot/pgsql/src/test/regress/expected/interval.out,v
retrieving revision 1.27
diff -c -r1.27 interval.out
*** src/test/regress/expected/interval.out    4 Apr 2009 04:53:25 -0000    1.27
--- src/test/regress/expected/interval.out    31 May 2009 22:13:35 -0000
***************
*** 472,486 ****
  LINE 1: SELECT interval '1 2' hour to minute;
                          ^
  SELECT interval '1 2:03' hour to minute;
!  interval
! ----------
!  02:03:00
  (1 row)

  SELECT interval '1 2:03:04' hour to minute;
!  interval
! ----------
!  02:03:00
  (1 row)

  SELECT interval '1 2' hour to second;
--- 472,486 ----
  LINE 1: SELECT interval '1 2' hour to minute;
                          ^
  SELECT interval '1 2:03' hour to minute;
!     interval
! ----------------
!  1 day 02:03:00
  (1 row)

  SELECT interval '1 2:03:04' hour to minute;
!     interval
! ----------------
!  1 day 02:03:00
  (1 row)

  SELECT interval '1 2' hour to second;
***************
*** 488,502 ****
  LINE 1: SELECT interval '1 2' hour to second;
                          ^
  SELECT interval '1 2:03' hour to second;
!  interval
! ----------
!  02:03:00
  (1 row)

  SELECT interval '1 2:03:04' hour to second;
!  interval
! ----------
!  02:03:04
  (1 row)

  SELECT interval '1 2' minute to second;
--- 488,502 ----
  LINE 1: SELECT interval '1 2' hour to second;
                          ^
  SELECT interval '1 2:03' hour to second;
!     interval
! ----------------
!  1 day 02:03:00
  (1 row)

  SELECT interval '1 2:03:04' hour to second;
!     interval
! ----------------
!  1 day 02:03:04
  (1 row)

  SELECT interval '1 2' minute to second;
***************
*** 504,518 ****
  LINE 1: SELECT interval '1 2' minute to second;
                          ^
  SELECT interval '1 2:03' minute to second;
!  interval
! ----------
!  00:02:03
  (1 row)

  SELECT interval '1 2:03:04' minute to second;
!  interval
! ----------
!  00:03:04
  (1 row)

  -- test syntaxes for restricted precision
--- 504,518 ----
  LINE 1: SELECT interval '1 2' minute to second;
                          ^
  SELECT interval '1 2:03' minute to second;
!     interval
! ----------------
!  1 day 00:02:03
  (1 row)

  SELECT interval '1 2:03:04' minute to second;
!     interval
! ----------------
!  1 day 02:03:04
  (1 row)

  -- test syntaxes for restricted precision
***************
*** 585,599 ****
  LINE 1: SELECT interval '1 2.345' hour to second(2);
                          ^
  SELECT interval '1 2:03.45678' hour to second(2);
!   interval
! -------------
!  00:02:03.46
  (1 row)

  SELECT interval '1 2:03:04.5678' hour to second(2);
!   interval
! -------------
!  02:03:04.57
  (1 row)

  SELECT interval '1 2.3456' minute to second(2);
--- 585,599 ----
  LINE 1: SELECT interval '1 2.345' hour to second(2);
                          ^
  SELECT interval '1 2:03.45678' hour to second(2);
!      interval
! -------------------
!  1 day 00:02:03.46
  (1 row)

  SELECT interval '1 2:03:04.5678' hour to second(2);
!      interval
! -------------------
!  1 day 02:03:04.57
  (1 row)

  SELECT interval '1 2.3456' minute to second(2);
***************
*** 601,615 ****
  LINE 1: SELECT interval '1 2.3456' minute to second(2);
                          ^
  SELECT interval '1 2:03.5678' minute to second(2);
!   interval
! -------------
!  00:02:03.57
  (1 row)

  SELECT interval '1 2:03:04.5678' minute to second(2);
!   interval
! -------------
!  00:03:04.57
  (1 row)

  -- test inputting and outputting SQL standard interval literals
--- 601,615 ----
  LINE 1: SELECT interval '1 2.3456' minute to second(2);
                          ^
  SELECT interval '1 2:03.5678' minute to second(2);
!      interval
! -------------------
!  1 day 00:02:03.57
  (1 row)

  SELECT interval '1 2:03:04.5678' minute to second(2);
!      interval
! -------------------
!  1 day 02:03:04.57
  (1 row)

  -- test inputting and outputting SQL standard interval literals