Thread: Macros for time magic values
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
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
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. +
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. +
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. +
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.
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
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
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
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. +
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. +
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
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
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. +
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
"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
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
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. +
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. +
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
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
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
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
"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
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
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>