Horton

Thread

  1. Macros for time magic values

    Bruce Momjian <bruce@momjian.us> — 2011-03-11T17:50:42Z

    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. +

  2. Re: Macros for time magic values

    Christopher Browne <cbbrowne@gmail.com> — 2011-03-11T17:58:01Z

    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

  3. Re: Macros for time magic values

    Bruce Momjian <bruce@momjian.us> — 2011-03-11T18:09:48Z

    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. +

  4. Re: Macros for time magic values

    Bruce Momjian <bruce@momjian.us> — 2011-03-11T18:12:10Z

    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. +

  5. Re: Macros for time magic values

    Bruce Momjian <bruce@momjian.us> — 2011-03-12T14:31:28Z

    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. +

  6. Re: Macros for time magic values

    Peter Eisentraut <peter_e@gmx.net> — 2011-03-12T20:29:03Z

    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.

  7. Re: Macros for time magic values

    Simon Riggs <simon@2ndquadrant.com> — 2011-03-14T08:22:59Z

    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

  8. Re: Macros for time magic values

    Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2011-03-14T09:58:58Z

    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

  9. Re: Macros for time magic values

    Robert Haas <robertmhaas@gmail.com> — 2011-03-14T13:25:28Z

    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

  10. Re: Macros for time magic values

    Andrew Dunstan <andrew@dunslane.net> — 2011-03-14T13:47:10Z

    
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

  11. Re: Macros for time magic values

    Bruce Momjian <bruce@momjian.us> — 2011-03-14T14:40:46Z

    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. +

  12. Re: Macros for time magic values

    Bruce Momjian <bruce@momjian.us> — 2011-03-14T14:42:46Z

    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. +

  13. Re: Macros for time magic values

    Simon Riggs <simon@2ndquadrant.com> — 2011-03-14T14:50:00Z

    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

  14. Re: Macros for time magic values

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-03-14T15:13:41Z

    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

  15. Re: Macros for time magic values

    Bruce Momjian <bruce@momjian.us> — 2011-03-14T15:21:15Z

    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'.  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 */

> 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. +

  16. Re: Macros for time magic values

    Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-03-14T15:56:31Z

    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

  17. Re: Macros for time magic values

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-03-14T16:01:36Z

    "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

  18. Re: Macros for time magic values

    Robert Haas <robertmhaas@gmail.com> — 2011-03-14T16:30:37Z

    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

  19. Re: Macros for time magic values

    Bruce Momjian <bruce@momjian.us> — 2011-03-14T16:36:36Z

    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. +

  20. Re: Macros for time magic values

    Bruce Momjian <bruce@momjian.us> — 2011-03-14T16:42:58Z

    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. +

  21. Re: Macros for time magic values

    Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-03-14T16:52:39Z

    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

  22. Re: Macros for time magic values

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-03-15T14:16:54Z

    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

  23. Re: Macros for time magic values

    Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-03-15T14:36:15Z

    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

  24. Re: Macros for time magic values

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-03-15T14:42:06Z

    "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

  25. Re: Macros for time magic values

    Alvaro Herrera <alvherre@commandprompt.com> — 2011-03-15T14:53:49Z

    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

  26. Re: Macros for time magic values

    Bruce Momjian <bruce@momjian.us> — 2011-09-05T19:38:46Z

    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. +