Thread: Macros for time magic values

Macros for time magic values

From
Bruce Momjian
Date:
It has bothered me that many of our time routines use special magic
constants for time values, e.g. 24, 12, 60, etc.

The attached patch changes these magic constants to macros to clarify
the code.  I would like to apply this for 9.1 as a cleanup.

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

  + It's impossible for everything to be true. +
diff --git a/src/backend/utils/adt/date.c b/src/backend/utils/adt/date.c
index 80dd10b..f96fa6c 100644
--- a/src/backend/utils/adt/date.c
+++ b/src/backend/utils/adt/date.c
@@ -2612,7 +2612,7 @@ timetz_zone(PG_FUNCTION_ARGS)
     type = DecodeSpecial(0, lowzone, &val);

     if (type == TZ || type == DTZ)
-        tz = val * 60;
+        tz = val * MINS_PER_HOUR;
     else
     {
         tzp = pg_tzset(tzname);
diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c
index 85f0206..f0fe2e3 100644
--- a/src/backend/utils/adt/datetime.c
+++ b/src/backend/utils/adt/datetime.c
@@ -342,7 +342,7 @@ j2date(int jd, int *year, int *month, int *day)
     *year = y - 4800;
     quad = julian * 2141 / 65536;
     *day = julian - 7834 * quad / 256;
-    *month = (quad + 10) % 12 + 1;
+    *month = (quad + 10) % MONTHS_PER_YEAR + 1;

     return;
 }    /* j2date() */
@@ -952,8 +952,8 @@ DecodeDateTime(char **field, int *ftype, int nf,
                  * DecodeTime()
                  */
                 /* test for > 24:00:00 */
-                if (tm->tm_hour > 24 ||
-                    (tm->tm_hour == 24 &&
+                if (tm->tm_hour > HOURS_PER_DAY ||
+                    (tm->tm_hour == HOURS_PER_DAY &&
                      (tm->tm_min > 0 || tm->tm_sec > 0 || *fsec > 0)))
                     return DTERR_FIELD_OVERFLOW;
                 break;
@@ -1371,12 +1371,12 @@ DecodeDateTime(char **field, int *ftype, int nf,
         return dterr;

     /* handle AM/PM */
-    if (mer != HR24 && tm->tm_hour > 12)
+    if (mer != HR24 && tm->tm_hour > HOURS_PER_DAY / 2)
         return DTERR_FIELD_OVERFLOW;
-    if (mer == AM && tm->tm_hour == 12)
+    if (mer == AM && tm->tm_hour == HOURS_PER_DAY / 2)
         tm->tm_hour = 0;
-    else if (mer == PM && tm->tm_hour != 12)
-        tm->tm_hour += 12;
+    else if (mer == PM && tm->tm_hour != HOURS_PER_DAY / 2)
+        tm->tm_hour += HOURS_PER_DAY / 2;

     /* do additional checking for full date specs... */
     if (*dtype == DTK_DATE)
@@ -2058,17 +2058,18 @@ DecodeTimeOnly(char **field, int *ftype, int nf,
         return dterr;

     /* handle AM/PM */
-    if (mer != HR24 && tm->tm_hour > 12)
+    if (mer != HR24 && tm->tm_hour > HOURS_PER_DAY / 2)
         return DTERR_FIELD_OVERFLOW;
-    if (mer == AM && tm->tm_hour == 12)
+    if (mer == AM && tm->tm_hour == HOURS_PER_DAY / 2)
         tm->tm_hour = 0;
-    else if (mer == PM && tm->tm_hour != 12)
-        tm->tm_hour += 12;
+    else if (mer == PM && tm->tm_hour != HOURS_PER_DAY / 2)
+        tm->tm_hour += HOURS_PER_DAY / 2;

-    if (tm->tm_hour < 0 || tm->tm_min < 0 || tm->tm_min > 59 ||
-        tm->tm_sec < 0 || tm->tm_sec > 60 || tm->tm_hour > 24 ||
+    if (tm->tm_hour < 0 || tm->tm_min < 0 || tm->tm_min > MINS_PER_HOUR - 1 ||
+        tm->tm_sec < 0 || tm->tm_sec > SECS_PER_MINUTE ||
+        tm->tm_hour > HOURS_PER_DAY ||
     /* test for > 24:00:00 */
-        (tm->tm_hour == 24 &&
+        (tm->tm_hour == HOURS_PER_DAY &&
          (tm->tm_min > 0 || tm->tm_sec > 0 || *fsec > 0)) ||
 #ifdef HAVE_INT64_TIMESTAMP
         *fsec < INT64CONST(0) || *fsec > USECS_PER_SEC
@@ -2396,13 +2397,15 @@ DecodeTime(char *str, int fmask, int range,

     /* do a sanity check */
 #ifdef HAVE_INT64_TIMESTAMP
-    if (tm->tm_hour < 0 || tm->tm_min < 0 || tm->tm_min > 59 ||
-        tm->tm_sec < 0 || tm->tm_sec > 60 || *fsec < INT64CONST(0) ||
+    if (tm->tm_hour < 0 || tm->tm_min < 0 || tm->tm_min > MINS_PER_HOUR -1 ||
+        tm->tm_sec < 0 || tm->tm_sec > SECS_PER_MINUTE ||
+        *fsec < INT64CONST(0) ||
         *fsec > USECS_PER_SEC)
         return DTERR_FIELD_OVERFLOW;
 #else
-    if (tm->tm_hour < 0 || tm->tm_min < 0 || tm->tm_min > 59 ||
-        tm->tm_sec < 0 || tm->tm_sec > 60 || *fsec < 0 || *fsec > 1)
+    if (tm->tm_hour < 0 || tm->tm_min < 0 || tm->tm_min > MINS_PER_HOUR - 1 ||
+        tm->tm_sec < 0 || tm->tm_sec > SECS_PER_MINUTE ||
+        *fsec < 0 || *fsec > 1)
         return DTERR_FIELD_OVERFLOW;
 #endif

@@ -2748,9 +2751,9 @@ DecodeTimezone(char *str, int *tzp)

     if (hr < 0 || hr > 14)
         return DTERR_TZDISP_OVERFLOW;
-    if (min < 0 || min >= 60)
+    if (min < 0 || min >= MINS_PER_HOUR)
         return DTERR_TZDISP_OVERFLOW;
-    if (sec < 0 || sec >= 60)
+    if (sec < 0 || sec >= SECS_PER_MINUTE)
         return DTERR_TZDISP_OVERFLOW;

     tz = (hr * MINS_PER_HOUR + min) * SECS_PER_MINUTE + sec;
@@ -3324,7 +3327,7 @@ DecodeISO8601Interval(char *str,
             {
                 case 'Y':
                     tm->tm_year += val;
-                    tm->tm_mon += (fval * 12);
+                    tm->tm_mon += (fval * MONTHS_PER_YEAR);
                     break;
                 case 'M':
                     tm->tm_mon += val;
@@ -3359,7 +3362,7 @@ DecodeISO8601Interval(char *str,
                         return DTERR_BAD_FORMAT;

                     tm->tm_year += val;
-                    tm->tm_mon += (fval * 12);
+                    tm->tm_mon += (fval * MONTHS_PER_YEAR);
                     if (unit == '\0')
                         return 0;
                     if (unit == 'T')
@@ -4155,7 +4158,7 @@ InstallTimeZoneAbbrevs(tzEntry *abbrevs, int n)
     {
         strncpy(newtbl[i].token, abbrevs[i].abbrev, TOKMAXLEN);
         newtbl[i].type = abbrevs[i].is_dst ? DTZ : TZ;
-        TOVAL(&newtbl[i], abbrevs[i].offset / 60);
+        TOVAL(&newtbl[i], abbrevs[i].offset / MINS_PER_HOUR);
     }

     /* Check the ordering, if testing */
diff --git a/src/backend/utils/adt/formatting.c b/src/backend/utils/adt/formatting.c
index f90d36d..aba1145 100644
--- a/src/backend/utils/adt/formatting.c
+++ b/src/backend/utils/adt/formatting.c
@@ -2129,7 +2129,7 @@ DCH_to_char(FormatNode *node, bool is_interval, TmToChar *in, char *out, Oid col
                  * intervals
                  */
                 sprintf(s, "%0*d", S_FM(n->suffix) ? 0 : 2,
-                        tm->tm_hour % (HOURS_PER_DAY / 2) == 0 ? 12 :
+                        tm->tm_hour % (HOURS_PER_DAY / 2) == 0 ? HOURS_PER_DAY / 2 :
                         tm->tm_hour % (HOURS_PER_DAY / 2));
                 if (S_THth(n->suffix))
                     str_numth(s, s, S_TH_TYPE(n->suffix));
@@ -2486,14 +2486,14 @@ DCH_to_char(FormatNode *node, bool is_interval, TmToChar *in, char *out, Oid col
                 if (!tm->tm_mon)
                     break;
                 sprintf(s, "%*s", S_FM(n->suffix) ? 0 : -4,
-                        rm_months_upper[12 - tm->tm_mon]);
+                        rm_months_upper[MONTHS_PER_YEAR - tm->tm_mon]);
                 s += strlen(s);
                 break;
             case DCH_rm:
                 if (!tm->tm_mon)
                     break;
                 sprintf(s, "%*s", S_FM(n->suffix) ? 0 : -4,
-                        rm_months_lower[12 - tm->tm_mon]);
+                        rm_months_lower[MONTHS_PER_YEAR - tm->tm_mon]);
                 s += strlen(s);
                 break;
             case DCH_W:
@@ -2779,12 +2779,12 @@ DCH_from_char(FormatNode *node, char *in, TmFromChar *out)
             case DCH_RM:
                 from_char_seq_search(&value, &s, rm_months_upper,
                                      ALL_UPPER, MAX_RM_LEN, n);
-                from_char_set_int(&out->mm, 12 - value, n);
+                from_char_set_int(&out->mm, MONTHS_PER_YEAR - value, n);
                 break;
             case DCH_rm:
                 from_char_seq_search(&value, &s, rm_months_lower,
                                      ALL_LOWER, MAX_RM_LEN, n);
-                from_char_set_int(&out->mm, 12 - value, n);
+                from_char_set_int(&out->mm, MONTHS_PER_YEAR - value, n);
                 break;
             case DCH_W:
                 from_char_parse_int(&out->w, &s, n);
@@ -3236,16 +3236,16 @@ do_to_timestamp(text *date_txt, text *fmt,

     if (tmfc.clock == CLOCK_12_HOUR)
     {
-        if (tm->tm_hour < 1 || tm->tm_hour > 12)
+        if (tm->tm_hour < 1 || tm->tm_hour > HOURS_PER_DAY / 2)
             ereport(ERROR,
                     (errcode(ERRCODE_INVALID_DATETIME_FORMAT),
                      errmsg("hour \"%d\" is invalid for the 12-hour clock",
                             tm->tm_hour),
                      errhint("Use the 24-hour clock, or give an hour between 1 and 12.")));

-        if (tmfc.pm && tm->tm_hour < 12)
-            tm->tm_hour += 12;
-        else if (!tmfc.pm && tm->tm_hour == 12)
+        if (tmfc.pm && tm->tm_hour < HOURS_PER_DAY / 2)
+            tm->tm_hour += HOURS_PER_DAY / 2;
+        else if (!tmfc.pm && tm->tm_hour == HOURS_PER_DAY / 2)
             tm->tm_hour = 0;
     }

@@ -3347,7 +3347,7 @@ do_to_timestamp(text *date_txt, text *fmt,

             y = ysum[isleap(tm->tm_year)];

-            for (i = 1; i <= 12; i++)
+            for (i = 1; i <= MONTHS_PER_YEAR; i++)
             {
                 if (tmfc.ddd < y[i])
                     break;
diff --git a/src/backend/utils/adt/nabstime.c b/src/backend/utils/adt/nabstime.c
index 0e25c5f..4b61b21 100644
--- a/src/backend/utils/adt/nabstime.c
+++ b/src/backend/utils/adt/nabstime.c
@@ -179,13 +179,13 @@ tm2abstime(struct pg_tm * tm, int tz)

     /* validate, before going out of range on some members */
     if (tm->tm_year < 1901 || tm->tm_year > 2038 ||
-        tm->tm_mon < 1 || tm->tm_mon > 12 ||
+        tm->tm_mon < 1 || tm->tm_mon > MONTHS_PER_YEAR ||
         tm->tm_mday < 1 || tm->tm_mday > 31 ||
         tm->tm_hour < 0 ||
-        tm->tm_hour > 24 ||        /* test for > 24:00:00 */
-        (tm->tm_hour == 24 && (tm->tm_min > 0 || tm->tm_sec > 0)) ||
-        tm->tm_min < 0 || tm->tm_min > 59 ||
-        tm->tm_sec < 0 || tm->tm_sec > 60)
+        tm->tm_hour > HOURS_PER_DAY ||        /* test for > 24:00:00 */
+        (tm->tm_hour == HOURS_PER_DAY && (tm->tm_min > 0 || tm->tm_sec > 0)) ||
+        tm->tm_min < 0 || tm->tm_min > MINS_PER_HOUR - 1 ||
+        tm->tm_sec < 0 || tm->tm_sec > SECS_PER_MINUTE)
         return INVALID_ABSTIME;

     day = date2j(tm->tm_year, tm->tm_mon, tm->tm_mday) - UNIX_EPOCH_JDATE;
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 1c2c160..45e7002 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -4422,7 +4422,7 @@ timestamp_zone(PG_FUNCTION_ARGS)

     if (type == TZ || type == DTZ)
     {
-        tz = -(val * 60);
+        tz = -(val * MINS_PER_HOUR);
         result = dt2local(timestamp, tz);
     }
     else
@@ -4596,7 +4596,7 @@ timestamptz_zone(PG_FUNCTION_ARGS)

     if (type == TZ || type == DTZ)
     {
-        tz = val * 60;
+        tz = val * MINS_PER_HOUR;
         result = dt2local(timestamp, tz);
     }
     else
diff --git a/src/include/utils/timestamp.h b/src/include/utils/timestamp.h
index 9e51b58..e14285f 100644
--- a/src/include/utils/timestamp.h
+++ b/src/include/utils/timestamp.h
@@ -81,6 +81,8 @@ typedef struct
  */
 #define DAYS_PER_MONTH    30        /* assumes exactly 30 days per month */
 #define HOURS_PER_DAY    24        /* assume no daylight savings time changes */
+#define MINS_PER_HOUR    60        /* assume no daylight savings time changes */
+#define SECS_PER_MINUTE    60        /* assume no daylight savings time changes */

 /*
  *    This doesn't adjust for uneven daylight savings time intervals or leap

Re: Macros for time magic values

From
Christopher Browne
Date:
On Fri, Mar 11, 2011 at 12:50 PM, Bruce Momjian <bruce@momjian.us> wrote:
> It has bothered me that many of our time routines use special magic
> constants for time values, e.g. 24, 12, 60, etc.
>
> The attached patch changes these magic constants to macros to clarify
> the code.  I would like to apply this for 9.1 as a cleanup.

The context diffs show off some references to 1901 and 2038...

Here's a *possible* extension to this...
--
http://linuxfinances.info/info/linuxdistributions.html

Attachment

Re: Macros for time magic values

From
Bruce Momjian
Date:
Christopher Browne wrote:
> On Fri, Mar 11, 2011 at 12:50 PM, Bruce Momjian <bruce@momjian.us> wrote:
> > It has bothered me that many of our time routines use special magic
> > constants for time values, e.g. 24, 12, 60, etc.
> >
> > The attached patch changes these magic constants to macros to clarify
> > the code. ?I would like to apply this for 9.1 as a cleanup.
> 
> The context diffs show off some references to 1901 and 2038...
> 
> Here's a *possible* extension to this...


Interesting idea, but UTIME_MINYEAR/UTIME_MAXYEAR is only defined in
src/interfaces/ecpg/pgtypeslib/dt.h, and it seems odd to duplicate or
move them for just one use site.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + It's impossible for everything to be true. +


Re: Macros for time magic values

From
Bruce Momjian
Date:
Bruce Momjian wrote:
> Christopher Browne wrote:
> > On Fri, Mar 11, 2011 at 12:50 PM, Bruce Momjian <bruce@momjian.us> wrote:
> > > It has bothered me that many of our time routines use special magic
> > > constants for time values, e.g. 24, 12, 60, etc.
> > >
> > > The attached patch changes these magic constants to macros to clarify
> > > the code. ?I would like to apply this for 9.1 as a cleanup.
> > 
> > The context diffs show off some references to 1901 and 2038...
> > 
> > Here's a *possible* extension to this...
> 
> 
> Interesting idea, but UTIME_MINYEAR/UTIME_MAXYEAR is only defined in
> src/interfaces/ecpg/pgtypeslib/dt.h, and it seems odd to duplicate or
> move them for just one use site.

We could move UTIME_MINYEAR/UTIME_MAXYEAR, but I don't see a common file
they both currently include.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + It's impossible for everything to be true. +


Re: Macros for time magic values

From
Bruce Momjian
Date:
Applied.

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

Bruce Momjian wrote:
> It has bothered me that many of our time routines use special magic
> constants for time values, e.g. 24, 12, 60, etc.
> 
> The attached patch changes these magic constants to macros to clarify
> the code.  I would like to apply this for 9.1 as a cleanup.
> 
> -- 
>   Bruce Momjian  <bruce@momjian.us>        http://momjian.us
>   EnterpriseDB                             http://enterprisedb.com
> 
>   + It's impossible for everything to be true. +

[ text/x-diff is unsupported, treating like TEXT/PLAIN ]

> diff --git a/src/backend/utils/adt/date.c b/src/backend/utils/adt/date.c
> index 80dd10b..f96fa6c 100644
> --- a/src/backend/utils/adt/date.c
> +++ b/src/backend/utils/adt/date.c
> @@ -2612,7 +2612,7 @@ timetz_zone(PG_FUNCTION_ARGS)
>      type = DecodeSpecial(0, lowzone, &val);
>  
>      if (type == TZ || type == DTZ)
> -        tz = val * 60;
> +        tz = val * MINS_PER_HOUR;
>      else
>      {
>          tzp = pg_tzset(tzname);
> diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c
> index 85f0206..f0fe2e3 100644
> --- a/src/backend/utils/adt/datetime.c
> +++ b/src/backend/utils/adt/datetime.c
> @@ -342,7 +342,7 @@ j2date(int jd, int *year, int *month, int *day)
>      *year = y - 4800;
>      quad = julian * 2141 / 65536;
>      *day = julian - 7834 * quad / 256;
> -    *month = (quad + 10) % 12 + 1;
> +    *month = (quad + 10) % MONTHS_PER_YEAR + 1;
>  
>      return;
>  }    /* j2date() */
> @@ -952,8 +952,8 @@ DecodeDateTime(char **field, int *ftype, int nf,
>                   * DecodeTime()
>                   */
>                  /* test for > 24:00:00 */
> -                if (tm->tm_hour > 24 ||
> -                    (tm->tm_hour == 24 &&
> +                if (tm->tm_hour > HOURS_PER_DAY ||
> +                    (tm->tm_hour == HOURS_PER_DAY &&
>                       (tm->tm_min > 0 || tm->tm_sec > 0 || *fsec > 0)))
>                      return DTERR_FIELD_OVERFLOW;
>                  break;
> @@ -1371,12 +1371,12 @@ DecodeDateTime(char **field, int *ftype, int nf,
>          return dterr;
>  
>      /* handle AM/PM */
> -    if (mer != HR24 && tm->tm_hour > 12)
> +    if (mer != HR24 && tm->tm_hour > HOURS_PER_DAY / 2)
>          return DTERR_FIELD_OVERFLOW;
> -    if (mer == AM && tm->tm_hour == 12)
> +    if (mer == AM && tm->tm_hour == HOURS_PER_DAY / 2)
>          tm->tm_hour = 0;
> -    else if (mer == PM && tm->tm_hour != 12)
> -        tm->tm_hour += 12;
> +    else if (mer == PM && tm->tm_hour != HOURS_PER_DAY / 2)
> +        tm->tm_hour += HOURS_PER_DAY / 2;
>  
>      /* do additional checking for full date specs... */
>      if (*dtype == DTK_DATE)
> @@ -2058,17 +2058,18 @@ DecodeTimeOnly(char **field, int *ftype, int nf,
>          return dterr;
>  
>      /* handle AM/PM */
> -    if (mer != HR24 && tm->tm_hour > 12)
> +    if (mer != HR24 && tm->tm_hour > HOURS_PER_DAY / 2)
>          return DTERR_FIELD_OVERFLOW;
> -    if (mer == AM && tm->tm_hour == 12)
> +    if (mer == AM && tm->tm_hour == HOURS_PER_DAY / 2)
>          tm->tm_hour = 0;
> -    else if (mer == PM && tm->tm_hour != 12)
> -        tm->tm_hour += 12;
> +    else if (mer == PM && tm->tm_hour != HOURS_PER_DAY / 2)
> +        tm->tm_hour += HOURS_PER_DAY / 2;
>  
> -    if (tm->tm_hour < 0 || tm->tm_min < 0 || tm->tm_min > 59 ||
> -        tm->tm_sec < 0 || tm->tm_sec > 60 || tm->tm_hour > 24 ||
> +    if (tm->tm_hour < 0 || tm->tm_min < 0 || tm->tm_min > MINS_PER_HOUR - 1 ||
> +        tm->tm_sec < 0 || tm->tm_sec > SECS_PER_MINUTE ||
> +        tm->tm_hour > HOURS_PER_DAY ||
>      /* test for > 24:00:00 */
> -        (tm->tm_hour == 24 &&
> +        (tm->tm_hour == HOURS_PER_DAY &&
>           (tm->tm_min > 0 || tm->tm_sec > 0 || *fsec > 0)) ||
>  #ifdef HAVE_INT64_TIMESTAMP
>          *fsec < INT64CONST(0) || *fsec > USECS_PER_SEC
> @@ -2396,13 +2397,15 @@ DecodeTime(char *str, int fmask, int range,
>  
>      /* do a sanity check */
>  #ifdef HAVE_INT64_TIMESTAMP
> -    if (tm->tm_hour < 0 || tm->tm_min < 0 || tm->tm_min > 59 ||
> -        tm->tm_sec < 0 || tm->tm_sec > 60 || *fsec < INT64CONST(0) ||
> +    if (tm->tm_hour < 0 || tm->tm_min < 0 || tm->tm_min > MINS_PER_HOUR -1 ||
> +        tm->tm_sec < 0 || tm->tm_sec > SECS_PER_MINUTE ||
> +        *fsec < INT64CONST(0) ||
>          *fsec > USECS_PER_SEC)
>          return DTERR_FIELD_OVERFLOW;
>  #else
> -    if (tm->tm_hour < 0 || tm->tm_min < 0 || tm->tm_min > 59 ||
> -        tm->tm_sec < 0 || tm->tm_sec > 60 || *fsec < 0 || *fsec > 1)
> +    if (tm->tm_hour < 0 || tm->tm_min < 0 || tm->tm_min > MINS_PER_HOUR - 1 ||
> +        tm->tm_sec < 0 || tm->tm_sec > SECS_PER_MINUTE ||
> +        *fsec < 0 || *fsec > 1)
>          return DTERR_FIELD_OVERFLOW;
>  #endif
>  
> @@ -2748,9 +2751,9 @@ DecodeTimezone(char *str, int *tzp)
>  
>      if (hr < 0 || hr > 14)
>          return DTERR_TZDISP_OVERFLOW;
> -    if (min < 0 || min >= 60)
> +    if (min < 0 || min >= MINS_PER_HOUR)
>          return DTERR_TZDISP_OVERFLOW;
> -    if (sec < 0 || sec >= 60)
> +    if (sec < 0 || sec >= SECS_PER_MINUTE)
>          return DTERR_TZDISP_OVERFLOW;
>  
>      tz = (hr * MINS_PER_HOUR + min) * SECS_PER_MINUTE + sec;
> @@ -3324,7 +3327,7 @@ DecodeISO8601Interval(char *str,
>              {
>                  case 'Y':
>                      tm->tm_year += val;
> -                    tm->tm_mon += (fval * 12);
> +                    tm->tm_mon += (fval * MONTHS_PER_YEAR);
>                      break;
>                  case 'M':
>                      tm->tm_mon += val;
> @@ -3359,7 +3362,7 @@ DecodeISO8601Interval(char *str,
>                          return DTERR_BAD_FORMAT;
>  
>                      tm->tm_year += val;
> -                    tm->tm_mon += (fval * 12);
> +                    tm->tm_mon += (fval * MONTHS_PER_YEAR);
>                      if (unit == '\0')
>                          return 0;
>                      if (unit == 'T')
> @@ -4155,7 +4158,7 @@ InstallTimeZoneAbbrevs(tzEntry *abbrevs, int n)
>      {
>          strncpy(newtbl[i].token, abbrevs[i].abbrev, TOKMAXLEN);
>          newtbl[i].type = abbrevs[i].is_dst ? DTZ : TZ;
> -        TOVAL(&newtbl[i], abbrevs[i].offset / 60);
> +        TOVAL(&newtbl[i], abbrevs[i].offset / MINS_PER_HOUR);
>      }
>  
>      /* Check the ordering, if testing */
> diff --git a/src/backend/utils/adt/formatting.c b/src/backend/utils/adt/formatting.c
> index f90d36d..aba1145 100644
> --- a/src/backend/utils/adt/formatting.c
> +++ b/src/backend/utils/adt/formatting.c
> @@ -2129,7 +2129,7 @@ DCH_to_char(FormatNode *node, bool is_interval, TmToChar *in, char *out, Oid col
>                   * intervals
>                   */
>                  sprintf(s, "%0*d", S_FM(n->suffix) ? 0 : 2,
> -                        tm->tm_hour % (HOURS_PER_DAY / 2) == 0 ? 12 :
> +                        tm->tm_hour % (HOURS_PER_DAY / 2) == 0 ? HOURS_PER_DAY / 2 :
>                          tm->tm_hour % (HOURS_PER_DAY / 2));
>                  if (S_THth(n->suffix))
>                      str_numth(s, s, S_TH_TYPE(n->suffix));
> @@ -2486,14 +2486,14 @@ DCH_to_char(FormatNode *node, bool is_interval, TmToChar *in, char *out, Oid col
>                  if (!tm->tm_mon)
>                      break;
>                  sprintf(s, "%*s", S_FM(n->suffix) ? 0 : -4,
> -                        rm_months_upper[12 - tm->tm_mon]);
> +                        rm_months_upper[MONTHS_PER_YEAR - tm->tm_mon]);
>                  s += strlen(s);
>                  break;
>              case DCH_rm:
>                  if (!tm->tm_mon)
>                      break;
>                  sprintf(s, "%*s", S_FM(n->suffix) ? 0 : -4,
> -                        rm_months_lower[12 - tm->tm_mon]);
> +                        rm_months_lower[MONTHS_PER_YEAR - tm->tm_mon]);
>                  s += strlen(s);
>                  break;
>              case DCH_W:
> @@ -2779,12 +2779,12 @@ DCH_from_char(FormatNode *node, char *in, TmFromChar *out)
>              case DCH_RM:
>                  from_char_seq_search(&value, &s, rm_months_upper,
>                                       ALL_UPPER, MAX_RM_LEN, n);
> -                from_char_set_int(&out->mm, 12 - value, n);
> +                from_char_set_int(&out->mm, MONTHS_PER_YEAR - value, n);
>                  break;
>              case DCH_rm:
>                  from_char_seq_search(&value, &s, rm_months_lower,
>                                       ALL_LOWER, MAX_RM_LEN, n);
> -                from_char_set_int(&out->mm, 12 - value, n);
> +                from_char_set_int(&out->mm, MONTHS_PER_YEAR - value, n);
>                  break;
>              case DCH_W:
>                  from_char_parse_int(&out->w, &s, n);
> @@ -3236,16 +3236,16 @@ do_to_timestamp(text *date_txt, text *fmt,
>  
>      if (tmfc.clock == CLOCK_12_HOUR)
>      {
> -        if (tm->tm_hour < 1 || tm->tm_hour > 12)
> +        if (tm->tm_hour < 1 || tm->tm_hour > HOURS_PER_DAY / 2)
>              ereport(ERROR,
>                      (errcode(ERRCODE_INVALID_DATETIME_FORMAT),
>                       errmsg("hour \"%d\" is invalid for the 12-hour clock",
>                              tm->tm_hour),
>                       errhint("Use the 24-hour clock, or give an hour between 1 and 12.")));
>  
> -        if (tmfc.pm && tm->tm_hour < 12)
> -            tm->tm_hour += 12;
> -        else if (!tmfc.pm && tm->tm_hour == 12)
> +        if (tmfc.pm && tm->tm_hour < HOURS_PER_DAY / 2)
> +            tm->tm_hour += HOURS_PER_DAY / 2;
> +        else if (!tmfc.pm && tm->tm_hour == HOURS_PER_DAY / 2)
>              tm->tm_hour = 0;
>      }
>  
> @@ -3347,7 +3347,7 @@ do_to_timestamp(text *date_txt, text *fmt,
>  
>              y = ysum[isleap(tm->tm_year)];
>  
> -            for (i = 1; i <= 12; i++)
> +            for (i = 1; i <= MONTHS_PER_YEAR; i++)
>              {
>                  if (tmfc.ddd < y[i])
>                      break;
> diff --git a/src/backend/utils/adt/nabstime.c b/src/backend/utils/adt/nabstime.c
> index 0e25c5f..4b61b21 100644
> --- a/src/backend/utils/adt/nabstime.c
> +++ b/src/backend/utils/adt/nabstime.c
> @@ -179,13 +179,13 @@ tm2abstime(struct pg_tm * tm, int tz)
>  
>      /* validate, before going out of range on some members */
>      if (tm->tm_year < 1901 || tm->tm_year > 2038 ||
> -        tm->tm_mon < 1 || tm->tm_mon > 12 ||
> +        tm->tm_mon < 1 || tm->tm_mon > MONTHS_PER_YEAR ||
>          tm->tm_mday < 1 || tm->tm_mday > 31 ||
>          tm->tm_hour < 0 ||
> -        tm->tm_hour > 24 ||        /* test for > 24:00:00 */
> -        (tm->tm_hour == 24 && (tm->tm_min > 0 || tm->tm_sec > 0)) ||
> -        tm->tm_min < 0 || tm->tm_min > 59 ||
> -        tm->tm_sec < 0 || tm->tm_sec > 60)
> +        tm->tm_hour > HOURS_PER_DAY ||        /* test for > 24:00:00 */
> +        (tm->tm_hour == HOURS_PER_DAY && (tm->tm_min > 0 || tm->tm_sec > 0)) ||
> +        tm->tm_min < 0 || tm->tm_min > MINS_PER_HOUR - 1 ||
> +        tm->tm_sec < 0 || tm->tm_sec > SECS_PER_MINUTE)
>          return INVALID_ABSTIME;
>  
>      day = date2j(tm->tm_year, tm->tm_mon, tm->tm_mday) - UNIX_EPOCH_JDATE;
> diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
> index 1c2c160..45e7002 100644
> --- a/src/backend/utils/adt/timestamp.c
> +++ b/src/backend/utils/adt/timestamp.c
> @@ -4422,7 +4422,7 @@ timestamp_zone(PG_FUNCTION_ARGS)
>  
>      if (type == TZ || type == DTZ)
>      {
> -        tz = -(val * 60);
> +        tz = -(val * MINS_PER_HOUR);
>          result = dt2local(timestamp, tz);
>      }
>      else
> @@ -4596,7 +4596,7 @@ timestamptz_zone(PG_FUNCTION_ARGS)
>  
>      if (type == TZ || type == DTZ)
>      {
> -        tz = val * 60;
> +        tz = val * MINS_PER_HOUR;
>          result = dt2local(timestamp, tz);
>      }
>      else
> diff --git a/src/include/utils/timestamp.h b/src/include/utils/timestamp.h
> index 9e51b58..e14285f 100644
> --- a/src/include/utils/timestamp.h
> +++ b/src/include/utils/timestamp.h
> @@ -81,6 +81,8 @@ typedef struct
>   */
>  #define DAYS_PER_MONTH    30        /* assumes exactly 30 days per month */
>  #define HOURS_PER_DAY    24        /* assume no daylight savings time changes */
> +#define MINS_PER_HOUR    60        /* assume no daylight savings time changes */
> +#define SECS_PER_MINUTE    60        /* assume no daylight savings time changes */
>  
>  /*
>   *    This doesn't adjust for uneven daylight savings time intervals or leap

> 
> -- 
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + It's impossible for everything to be true. +


Re: Macros for time magic values

From
Peter Eisentraut
Date:
On fre, 2011-03-11 at 12:50 -0500, Bruce Momjian wrote:
> It has bothered me that many of our time routines use special magic
> constants for time values, e.g. 24, 12, 60, etc.
> 
> The attached patch changes these magic constants to macros to clarify
> the code.  I would like to apply this for 9.1 as a cleanup.

I think it's much clearer with the plain numbers.



Re: Macros for time magic values

From
Simon Riggs
Date:
On Sat, 2011-03-12 at 22:29 +0200, Peter Eisentraut wrote:
> On fre, 2011-03-11 at 12:50 -0500, Bruce Momjian wrote:
> > It has bothered me that many of our time routines use special magic
> > constants for time values, e.g. 24, 12, 60, etc.
> > 
> > The attached patch changes these magic constants to macros to clarify
> > the code.  I would like to apply this for 9.1 as a cleanup.
> 
> I think it's much clearer with the plain numbers.

Yeh. It's not like the values 24, 12 or 60 were going to change.

-- Simon Riggs           http://www.2ndQuadrant.com/books/PostgreSQL Development, 24x7 Support, Training and Services



Re: Macros for time magic values

From
Robert Haas
Date:
On Mon, Mar 14, 2011 at 4:22 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
> On Sat, 2011-03-12 at 22:29 +0200, Peter Eisentraut wrote:
>> On fre, 2011-03-11 at 12:50 -0500, Bruce Momjian wrote:
>> > It has bothered me that many of our time routines use special magic
>> > constants for time values, e.g. 24, 12, 60, etc.
>> >
>> > The attached patch changes these magic constants to macros to clarify
>> > the code.  I would like to apply this for 9.1 as a cleanup.
>>
>> I think it's much clearer with the plain numbers.
>
> Yeh. It's not like the values 24, 12 or 60 were going to change.

I had the same thought.  OTOH, even in 9.0 we have constants for
BITS_PER_BYTE, DAYS_PER_YEAR (365.25), MONTHS_PER_YEAR, DAYS_PER_MONTH
(30, as it turns out), HOURS_PER_DAY, SECS_PER_YEAR (that's a
constant?), SECS_PER_DAY, SECS_PER_HOUR, MINS_PER_HOUR, USECS_PER_DAY,
USECS_PER_HOUR, USECS_PER_MINUTE, and USECS_PER_SEC.  And there's no
real reason to use those symbols in only some of the contexts where
they are relevant.

So my only real gripe with this patch is that Bruce appears to have
added duplicate definitions of SECS_PER_MINUTE and MINS_PER_HOUR to
timestamp.h.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: Macros for time magic values

From
Andrew Dunstan
Date:

On 03/14/2011 09:25 AM, Robert Haas wrote:
> On Mon, Mar 14, 2011 at 4:22 AM, Simon Riggs<simon@2ndquadrant.com>  wrote:
>> On Sat, 2011-03-12 at 22:29 +0200, Peter Eisentraut wrote:
>>> On fre, 2011-03-11 at 12:50 -0500, Bruce Momjian wrote:
>>>> It has bothered me that many of our time routines use special magic
>>>> constants for time values, e.g. 24, 12, 60, etc.
>>>>
>>>> The attached patch changes these magic constants to macros to clarify
>>>> the code.  I would like to apply this for 9.1 as a cleanup.
>>> I think it's much clearer with the plain numbers.
>> Yeh. It's not like the values 24, 12 or 60 were going to change.
> I had the same thought.


I think the advantage is that it's much harder to mistype a symbolic 
constant and not have it noticed than to mistype a number literal and 
not have it noticed. It's debatable whether this advantage is worth the 
loss in clarity in the case of very well known values such as seconds 
per minute.

cheers

andrew


Re: Macros for time magic values

From
Bruce Momjian
Date:
Robert Haas wrote:
> On Mon, Mar 14, 2011 at 4:22 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
> > On Sat, 2011-03-12 at 22:29 +0200, Peter Eisentraut wrote:
> >> On fre, 2011-03-11 at 12:50 -0500, Bruce Momjian wrote:
> >> > It has bothered me that many of our time routines use special magic
> >> > constants for time values, e.g. 24, 12, 60, etc.
> >> >
> >> > The attached patch changes these magic constants to macros to clarify
> >> > the code. ?I would like to apply this for 9.1 as a cleanup.
> >>
> >> I think it's much clearer with the plain numbers.
> >
> > Yeh. It's not like the values 24, 12 or 60 were going to change.
> 
> I had the same thought.  OTOH, even in 9.0 we have constants for
> BITS_PER_BYTE, DAYS_PER_YEAR (365.25), MONTHS_PER_YEAR, DAYS_PER_MONTH
> (30, as it turns out), HOURS_PER_DAY, SECS_PER_YEAR (that's a
> constant?), SECS_PER_DAY, SECS_PER_HOUR, MINS_PER_HOUR, USECS_PER_DAY,
> USECS_PER_HOUR, USECS_PER_MINUTE, and USECS_PER_SEC.  And there's no
> real reason to use those symbols in only some of the contexts where
> they are relevant.
> 
> So my only real gripe with this patch is that Bruce appears to have
> added duplicate definitions of SECS_PER_MINUTE and MINS_PER_HOUR to
> timestamp.h.

Good catch, duplicates removed.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + It's impossible for everything to be true. +


Re: Macros for time magic values

From
Bruce Momjian
Date:
Simon Riggs wrote:
> On Sat, 2011-03-12 at 22:29 +0200, Peter Eisentraut wrote:
> > On fre, 2011-03-11 at 12:50 -0500, Bruce Momjian wrote:
> > > It has bothered me that many of our time routines use special magic
> > > constants for time values, e.g. 24, 12, 60, etc.
> > > 
> > > The attached patch changes these magic constants to macros to clarify
> > > the code.  I would like to apply this for 9.1 as a cleanup.
> > 
> > I think it's much clearer with the plain numbers.
> 
> Yeh. It's not like the values 24, 12 or 60 were going to change.

Well, I find the macro names clearer because it removes domain-specific
knowledge from code that is already complex, e.g '24' represents what?
It is similar to the argument of why use C when you can use assembler. 
I find the macros a pure code clarity win.  If most people don't we
should remove them consistently.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + It's impossible for everything to be true. +


Re: Macros for time magic values

From
Simon Riggs
Date:
On Mon, 2011-03-14 at 10:42 -0400, Bruce Momjian wrote:
> Simon Riggs wrote:
> > On Sat, 2011-03-12 at 22:29 +0200, Peter Eisentraut wrote:
> > > On fre, 2011-03-11 at 12:50 -0500, Bruce Momjian wrote:
> > > > It has bothered me that many of our time routines use special magic
> > > > constants for time values, e.g. 24, 12, 60, etc.
> > > > 
> > > > The attached patch changes these magic constants to macros to clarify
> > > > the code.  I would like to apply this for 9.1 as a cleanup.
> > > 
> > > I think it's much clearer with the plain numbers.
> > 
> > Yeh. It's not like the values 24, 12 or 60 were going to change.
> 
> Well, I find the macro names clearer because it removes domain-specific
> knowledge from code that is already complex, e.g '24' represents what?
> It is similar to the argument of why use C when you can use assembler. 
> I find the macros a pure code clarity win.  If most people don't we
> should remove them consistently.

PGSQL_EMOTION_GOOD_ARGUMENT

-- Simon Riggs           http://www.2ndQuadrant.com/books/PostgreSQL Development, 24x7 Support, Training and Services



Re: Macros for time magic values

From
Tom Lane
Date:
Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Mar 14, 2011 at 4:22 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
>> On Sat, 2011-03-12 at 22:29 +0200, Peter Eisentraut wrote:
>>> I think it's much clearer with the plain numbers.

>> Yeh. It's not like the values 24, 12 or 60 were going to change.

> I had the same thought.  OTOH, even in 9.0 we have constants for
> BITS_PER_BYTE, DAYS_PER_YEAR (365.25), MONTHS_PER_YEAR, DAYS_PER_MONTH
> (30, as it turns out), HOURS_PER_DAY, SECS_PER_YEAR (that's a
> constant?), SECS_PER_DAY, SECS_PER_HOUR, MINS_PER_HOUR, USECS_PER_DAY,
> USECS_PER_HOUR, USECS_PER_MINUTE, and USECS_PER_SEC.  And there's no
> real reason to use those symbols in only some of the contexts where
> they are relevant.

Well, those existing symbols are there because Bruce put them in in
previous iterations of this same sort of patch.  And as you note,
some of them are pretty darn questionable because the underlying
number *isn't* as well defined as all that.

If Bruce is the only person who finds this to be a readability
improvement, maybe we should think about backing all of those
changes out.
        regards, tom lane


Re: Macros for time magic values

From
Bruce Momjian
Date:
Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Mon, Mar 14, 2011 at 4:22 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
> >> On Sat, 2011-03-12 at 22:29 +0200, Peter Eisentraut wrote:
> >>> I think it's much clearer with the plain numbers.
> 
> >> Yeh. It's not like the values 24, 12 or 60 were going to change.
> 
> > I had the same thought.  OTOH, even in 9.0 we have constants for
> > BITS_PER_BYTE, DAYS_PER_YEAR (365.25), MONTHS_PER_YEAR, DAYS_PER_MONTH
> > (30, as it turns out), HOURS_PER_DAY, SECS_PER_YEAR (that's a
> > constant?), SECS_PER_DAY, SECS_PER_HOUR, MINS_PER_HOUR, USECS_PER_DAY,
> > USECS_PER_HOUR, USECS_PER_MINUTE, and USECS_PER_SEC.  And there's no
> > real reason to use those symbols in only some of the contexts where
> > they are relevant.
> 
> Well, those existing symbols are there because Bruce put them in in
> previous iterations of this same sort of patch.  And as you note,

Right.

> some of them are pretty darn questionable because the underlying
> number *isn't* as well defined as all that.

The macro does allow us to centralize comments on their imprecision,
e.g.:
/* *  DAYS_PER_MONTH is very imprecise.  The more accurate value is *  365.2425/12 = 30.436875, or '30 days 10:29:06'.
Rightnow we only *  return an integral number of days, but someday perhaps we should *  also return a 'time' value to
beused as well.  ISO 8601 suggests *  30 days. */#define DAYS_PER_MONTH  30      /* assumes exactly 30 days per month
*/

> If Bruce is the only person who finds this to be a readability
> improvement, maybe we should think about backing all of those
> changes out.

Yes, it should be done consistently.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + It's impossible for everything to be true. +


Re: Macros for time magic values

From
"Kevin Grittner"
Date:
Bruce Momjian <bruce@momjian.us> wrote:
> Tom Lane wrote:
>> some of them are pretty darn questionable because the underlying
>> number *isn't* as well defined as all that.
> 
> The macro does allow us to centralize comments on their
> imprecision,
> e.g.:
> 
> /*
>  *  DAYS_PER_MONTH is very imprecise.  The more accurate value is
>  *  365.2425/12 = 30.436875, or '30 days 10:29:06'.  Right now we
>  *  only return an integral number of days, but someday perhaps we
>  *  should also return a 'time' value to be used as well.
>  *  ISO 8601 suggests 30 days.
>  */
> #define DAYS_PER_MONTH  30      /* assumes exactly 30 days per
>                                  * month */
My first reaction that this change was about a net wash in
readability for me -- in a couple places it might save me a few
moments thinking about what the number was meant to represent,
balanced against following the ctag back to the #define to see what
number was used for things like DAYS_PER_YEAR or DAYS_PER_MONTH.
Comments like the one Bruce cites above seem like they tip the
scales in favor of the patch for me.  Having a place to document
the choice of questionable values seems like it's better than just
using the questionable values "bare" all over the place.  Neither
omission of the justification nor repeating it seems better.
-Kevin


Re: Macros for time magic values

From
Tom Lane
Date:
"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
> My first reaction that this change was about a net wash in
> readability for me -- in a couple places it might save me a few
> moments thinking about what the number was meant to represent,
> balanced against following the ctag back to the #define to see what
> number was used for things like DAYS_PER_YEAR or DAYS_PER_MONTH.
> Comments like the one Bruce cites above seem like they tip the
> scales in favor of the patch for me.  Having a place to document
> the choice of questionable values seems like it's better than just
> using the questionable values "bare" all over the place.  Neither
> omission of the justification nor repeating it seems better.

Another advantage of the macros is that it makes it a lot easier to grep
to see where a questionable value is being used.  Originally I'd felt
that wrapping those bogus numbers in macros was a bad idea, but the
documentation and searching advantages are enough to make me think it's
all right.
        regards, tom lane


Re: Macros for time magic values

From
Robert Haas
Date:
On Mon, Mar 14, 2011 at 12:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> "Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
>> My first reaction that this change was about a net wash in
>> readability for me -- in a couple places it might save me a few
>> moments thinking about what the number was meant to represent,
>> balanced against following the ctag back to the #define to see what
>> number was used for things like DAYS_PER_YEAR or DAYS_PER_MONTH.
>
>> Comments like the one Bruce cites above seem like they tip the
>> scales in favor of the patch for me.  Having a place to document
>> the choice of questionable values seems like it's better than just
>> using the questionable values "bare" all over the place.  Neither
>> omission of the justification nor repeating it seems better.
>
> Another advantage of the macros is that it makes it a lot easier to grep
> to see where a questionable value is being used.  Originally I'd felt
> that wrapping those bogus numbers in macros was a bad idea, but the
> documentation and searching advantages are enough to make me think it's
> all right.

Yeah, I agree.  And I do think that there is also some value of having
constants for SECS_PER_MINUTE and MINUTES_PER_HOUR, because otherwise
it can be unclear what 60 means in a particular context.   We're at
the high end of what I consider reasonable in terms of defining
constants to represent values that aren't likely to change, but there
is tangible value in being able to grep for those constants when
you're trying to figure out what things might need changing, or just
to understand the code better.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: Macros for time magic values

From
Bruce Momjian
Date:
Robert Haas wrote:
> On Mon, Mar 14, 2011 at 12:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > "Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
> >> My first reaction that this change was about a net wash in
> >> readability for me -- in a couple places it might save me a few
> >> moments thinking about what the number was meant to represent,
> >> balanced against following the ctag back to the #define to see what
> >> number was used for things like DAYS_PER_YEAR or DAYS_PER_MONTH.
> >
> >> Comments like the one Bruce cites above seem like they tip the
> >> scales in favor of the patch for me. ?Having a place to document
> >> the choice of questionable values seems like it's better than just
> >> using the questionable values "bare" all over the place. ?Neither
> >> omission of the justification nor repeating it seems better.
> >
> > Another advantage of the macros is that it makes it a lot easier to grep
> > to see where a questionable value is being used. ?Originally I'd felt
> > that wrapping those bogus numbers in macros was a bad idea, but the
> > documentation and searching advantages are enough to make me think it's
> > all right.
> 
> Yeah, I agree.  And I do think that there is also some value of having
> constants for SECS_PER_MINUTE and MINUTES_PER_HOUR, because otherwise
> it can be unclear what 60 means in a particular context.   We're at
> the high end of what I consider reasonable in terms of defining
> constants to represent values that aren't likely to change, but there
> is tangible value in being able to grep for those constants when
> you're trying to figure out what things might need changing, or just
> to understand the code better.

Yes, I did have to study the code to figure out which to use:
   if (type == TZ || type == DTZ)   {       tz = -(val * MINS_PER_HOUR);       result = dt2local(timestamp, tz);   }

We measure timezone differences in minutes.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + It's impossible for everything to be true. +


Re: Macros for time magic values

From
Bruce Momjian
Date:
Bruce Momjian wrote:
> > Yeah, I agree.  And I do think that there is also some value of having
> > constants for SECS_PER_MINUTE and MINUTES_PER_HOUR, because otherwise
> > it can be unclear what 60 means in a particular context.   We're at
> > the high end of what I consider reasonable in terms of defining
> > constants to represent values that aren't likely to change, but there
> > is tangible value in being able to grep for those constants when
> > you're trying to figure out what things might need changing, or just
> > to understand the code better.
> 
> Yes, I did have to study the code to figure out which to use:
> 
>     if (type == TZ || type == DTZ)
>     {
>         tz = -(val * MINS_PER_HOUR);
>         result = dt2local(timestamp, tz);
>     }
> 
> We measure timezone differences in minutes.

Let me also add that I realize I am often a royal pain in the neck. 

There is no smiley on that --- I know I am often chaotic in how I
approach things and push them forward.  I do my best, but that is often
the outcome.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + It's impossible for everything to be true. +


Re: Macros for time magic values

From
"Kevin Grittner"
Date:
Bruce Momjian <bruce@momjian.us> wrote:
> Let me also add that I realize I am often a royal pain in the
> neck. 
It really stands out compared to all the timid shrinking violets who
post here.  ;-)
Seriously, I've always found that a group works best with a mix of
personalities with their various approaches, strengths, and
weaknesses.  You more than carry your weight -- by a long shot.  I
see no reason for you to apologize or fret over your approach.
-Kevin


Re: Macros for time magic values

From
Dimitri Fontaine
Date:
Simon Riggs <simon@2ndQuadrant.com> writes:

> On Sat, 2011-03-12 at 22:29 +0200, Peter Eisentraut wrote:
>> On fre, 2011-03-11 at 12:50 -0500, Bruce Momjian wrote:
>> > It has bothered me that many of our time routines use special magic
>> > constants for time values, e.g. 24, 12, 60, etc.
>> > 
>> > The attached patch changes these magic constants to macros to clarify
>> > the code.  I would like to apply this for 9.1 as a cleanup.
>> 
>> I think it's much clearer with the plain numbers.
>
> Yeh. It's not like the values 24, 12 or 60 were going to change.

Would it help moving toward Leap Second support, and is this something
we want to have?
 http://en.wikipedia.org/wiki/Leap_second

Regards,
-- 
Dimitri Fontaine
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support


Re: Macros for time magic values

From
Tom Lane
Date:
Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:
> Would it help moving toward Leap Second support, and is this something
> we want to have?

IMO we don't want to have that, as it would completely bollix datetime
calculations of all kinds.  You couldn't even count on stored timestamps
not changing their meaning.
        regards, tom lane


Re: Macros for time magic values

From
"Kevin Grittner"
Date:
Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:
>> Would it help moving toward Leap Second support, and is this
>> something we want to have?
> 
> IMO we don't want to have that, as it would completely bollix
> datetime calculations of all kinds.  You couldn't even count on
> stored timestamps not changing their meaning.
I'm inclined to agree, but if that's the choice, should we stop
claiming that we're using UTC, and instead claim UT1 support?  It
always seemed a little odd to me that the docs say UTC but there's
no actual support for leap seconds in calculations.
-Kevin


Re: Macros for time magic values

From
Tom Lane
Date:
"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
> Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:
>>> Would it help moving toward Leap Second support, and is this
>>> something we want to have?

>> IMO we don't want to have that, as it would completely bollix
>> datetime calculations of all kinds.  You couldn't even count on
>> stored timestamps not changing their meaning.
> I'm inclined to agree, but if that's the choice, should we stop
> claiming that we're using UTC, and instead claim UT1 support?  It
> always seemed a little odd to me that the docs say UTC but there's
> no actual support for leap seconds in calculations.

Maybe, but if the docs started talking about that, we'd have to define
the term every time.  The number of PG users who know what UT1 is can
probably be counted without running out of toes.
        regards, tom lane


Re: Macros for time magic values

From
Alvaro Herrera
Date:
Excerpts from Tom Lane's message of mar mar 15 11:42:06 -0300 2011:
> "Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
> > Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:
> >>> Would it help moving toward Leap Second support, and is this
> >>> something we want to have?
> 
> >> IMO we don't want to have that, as it would completely bollix
> >> datetime calculations of all kinds.  You couldn't even count on
> >> stored timestamps not changing their meaning.
>  
> > I'm inclined to agree, but if that's the choice, should we stop
> > claiming that we're using UTC, and instead claim UT1 support?  It
> > always seemed a little odd to me that the docs say UTC but there's
> > no actual support for leap seconds in calculations.
> 
> Maybe, but if the docs started talking about that, we'd have to define
> the term every time.  The number of PG users who know what UT1 is can
> probably be counted without running out of toes.

A small note somewhere visible would suffice: "these docs talk about UTC
but they really mean UT1 because we have no leap seconds support".

-- 
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support


Re: Macros for time magic values

From
Bruce Momjian
Date:
Alvaro Herrera wrote:
> Excerpts from Tom Lane's message of mar mar 15 11:42:06 -0300 2011:
> > "Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
> > > Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > >> Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:
> > >>> Would it help moving toward Leap Second support, and is this
> > >>> something we want to have?
> >
> > >> IMO we don't want to have that, as it would completely bollix
> > >> datetime calculations of all kinds.  You couldn't even count on
> > >> stored timestamps not changing their meaning.
> >
> > > I'm inclined to agree, but if that's the choice, should we stop
> > > claiming that we're using UTC, and instead claim UT1 support?  It
> > > always seemed a little odd to me that the docs say UTC but there's
> > > no actual support for leap seconds in calculations.
> >
> > Maybe, but if the docs started talking about that, we'd have to define
> > the term every time.  The number of PG users who know what UT1 is can
> > probably be counted without running out of toes.
>
> A small note somewhere visible would suffice: "these docs talk about UTC
> but they really mean UT1 because we have no leap seconds support".

Done with the attached doc patch, backpatched to 9.1.

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

  + It's impossible for everything to be true. +
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
new file mode 100644
index 6d5dad3..d6baf84
*** a/doc/src/sgml/catalogs.sgml
--- b/doc/src/sgml/catalogs.sgml
***************
*** 8341,8347 ****
     The view <structname>pg_timezone_names</structname> provides a list
     of time zone names that are recognized by <command>SET TIMEZONE</>,
     along with their associated abbreviations, UTC offsets,
!    and daylight-savings status.
     Unlike the abbreviations shown in <link
     linkend="view-pg-timezone-abbrevs"><structname>pg_timezone_abbrevs</structname></link>, many of these names imply
aset of daylight-savings transition 
     date rules.  Therefore, the associated information changes across local DST
--- 8341,8349 ----
     The view <structname>pg_timezone_names</structname> provides a list
     of time zone names that are recognized by <command>SET TIMEZONE</>,
     along with their associated abbreviations, UTC offsets,
!    and daylight-savings status.  (Technically,
!    <productname>PostgreSQL</productname> uses <acronym>UT1</> rather
!    than UTC because leap seconds are not handled.)
     Unlike the abbreviations shown in <link
     linkend="view-pg-timezone-abbrevs"><structname>pg_timezone_abbrevs</structname></link>, many of these names imply
aset of daylight-savings transition 
     date rules.  Therefore, the associated information changes across local DST
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
new file mode 100644
index 4c3e232..c03dd6c
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
*************** SELECT EXTRACT(SECOND FROM TIME '17:12:2
*** 6898,6904 ****
         <para>
          The time zone offset from UTC, measured in seconds.  Positive values
          correspond to time zones east of UTC, negative values to
!         zones west of UTC.
         </para>
        </listitem>
       </varlistentry>
--- 6898,6906 ----
         <para>
          The time zone offset from UTC, measured in seconds.  Positive values
          correspond to time zones east of UTC, negative values to
!         zones west of UTC.  (Technically,
!         <productname>PostgreSQL</productname> uses <acronym>UT1</> because
!         leap seconds are not handled.)
         </para>
        </listitem>
       </varlistentry>