Re: [GENERAL] INTERVAL data type and libpq - what format?

I wrote:
> In a related example,

> regression=# select interval '123 11' day;
>  interval
> ----------
>  134 days
> (1 row)

> we seem to be adding the 123 and 11 together.  This is, um,
> surprising behavior ... I'd be inclined to think throwing an
> error is more appropriate.

I looked into this and found out why the code wasn't throwing an error
already: it seems to be a case of ancient bit-shaving.  DecodeInterval
maintains an integer bitmask of field types it's seen already, and
reports an error if a new field's bit is already set in that mask.
However, DAY and WEEK are coded to share a bit, as are YEAR, DECADE,
CENTURY, and MILLENIUM; which means the code can't throw error for
multiple occurrences of any one of those field types.  However, there
is room in the mask for all these fields to have their own bits, so
the attached proposed patch just does that.

The potential objections I can see to this patch are:

(1) it eats more than half of the remaining bits in the DTK_M() mask.
This doesn't seem to me to be a big problem --- it's not like people
are likely to come up with many more time intervals for us to support.
Also we could probably shave some bits by eliminating redundancies
if we had to.  (For instance, I think IGNORE_DTF could safely be given
a code above 31 since there's no need for it to be in the bitmasks.)

(2) WEEK, DECADE, CENTURY, and MILLENNIUM are too generic to be used
as #define names without some kind of prefix or suffix.  This is a real
risk, but considering the other #defines in this list have no prefix or
suffix, it seems inconsistent to put one on these.  The buildfarm should
tell us soon enough if there's a real problem (the code does compile
cleanly for me).  If we do have a problem, we should add a prefix/suffix
to all these macros at once; but I'd rather not do that as part of a
small bugfix.

Comments?

            regards, tom lane

Index: src/backend/utils/adt/datetime.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/adt/datetime.c,v
retrieving revision 1.205
diff -c -r1.205 datetime.c
*** src/backend/utils/adt/datetime.c    26 May 2009 02:17:50 -0000    1.205
--- src/backend/utils/adt/datetime.c    31 May 2009 23:45:06 -0000
***************
*** 3022,3040 ****
                          tm->tm_hour += val;
                          AdjustFractSeconds(fval, tm, fsec, SECS_PER_HOUR);
                          tmask = DTK_M(HOUR);
!                         type = DTK_DAY;
                          break;

                      case DTK_DAY:
                          tm->tm_mday += val;
                          AdjustFractSeconds(fval, tm, fsec, SECS_PER_DAY);
!                         tmask = (fmask & DTK_M(DAY)) ? 0 : DTK_M(DAY);
                          break;

                      case DTK_WEEK:
                          tm->tm_mday += val * 7;
                          AdjustFractDays(fval, tm, fsec, 7);
!                         tmask = (fmask & DTK_M(DAY)) ? 0 : DTK_M(DAY);
                          break;

                      case DTK_MONTH:
--- 3022,3040 ----
                          tm->tm_hour += val;
                          AdjustFractSeconds(fval, tm, fsec, SECS_PER_HOUR);
                          tmask = DTK_M(HOUR);
!                         type = DTK_DAY;    /* set for next field */
                          break;

                      case DTK_DAY:
                          tm->tm_mday += val;
                          AdjustFractSeconds(fval, tm, fsec, SECS_PER_DAY);
!                         tmask = DTK_M(DAY);
                          break;

                      case DTK_WEEK:
                          tm->tm_mday += val * 7;
                          AdjustFractDays(fval, tm, fsec, 7);
!                         tmask = DTK_M(WEEK);
                          break;

                      case DTK_MONTH:
***************
*** 3047,3074 ****
                          tm->tm_year += val;
                          if (fval != 0)
                              tm->tm_mon += fval * MONTHS_PER_YEAR;
!                         tmask = (fmask & DTK_M(YEAR)) ? 0 : DTK_M(YEAR);
                          break;

                      case DTK_DECADE:
                          tm->tm_year += val * 10;
                          if (fval != 0)
                              tm->tm_mon += fval * MONTHS_PER_YEAR * 10;
!                         tmask = (fmask & DTK_M(YEAR)) ? 0 : DTK_M(YEAR);
                          break;

                      case DTK_CENTURY:
                          tm->tm_year += val * 100;
                          if (fval != 0)
                              tm->tm_mon += fval * MONTHS_PER_YEAR * 100;
!                         tmask = (fmask & DTK_M(YEAR)) ? 0 : DTK_M(YEAR);
                          break;

                      case DTK_MILLENNIUM:
                          tm->tm_year += val * 1000;
                          if (fval != 0)
                              tm->tm_mon += fval * MONTHS_PER_YEAR * 1000;
!                         tmask = (fmask & DTK_M(YEAR)) ? 0 : DTK_M(YEAR);
                          break;

                      default:
--- 3047,3074 ----
                          tm->tm_year += val;
                          if (fval != 0)
                              tm->tm_mon += fval * MONTHS_PER_YEAR;
!                         tmask = DTK_M(YEAR);
                          break;

                      case DTK_DECADE:
                          tm->tm_year += val * 10;
                          if (fval != 0)
                              tm->tm_mon += fval * MONTHS_PER_YEAR * 10;
!                         tmask = DTK_M(DECADE);
                          break;

                      case DTK_CENTURY:
                          tm->tm_year += val * 100;
                          if (fval != 0)
                              tm->tm_mon += fval * MONTHS_PER_YEAR * 100;
!                         tmask = DTK_M(CENTURY);
                          break;

                      case DTK_MILLENNIUM:
                          tm->tm_year += val * 1000;
                          if (fval != 0)
                              tm->tm_mon += fval * MONTHS_PER_YEAR * 1000;
!                         tmask = DTK_M(MILLENNIUM);
                          break;

                      default:
Index: src/include/utils/datetime.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/utils/datetime.h,v
retrieving revision 1.73
diff -c -r1.73 datetime.h
*** src/include/utils/datetime.h    26 May 2009 02:17:50 -0000    1.73
--- src/include/utils/datetime.h    31 May 2009 23:45:06 -0000
***************
*** 114,119 ****
--- 114,124 ----
  /* generic fields to help with parsing */
  #define ISODATE 22
  #define ISOTIME 23
+ /* these are only for parsing intervals */
+ #define WEEK        24
+ #define DECADE        25
+ #define CENTURY        26
+ #define MILLENNIUM    27
  /* reserved for unrecognized string values */
  #define UNKNOWN_FIELD    31