Re: [HACKERS] Macros for time magic values
Alvaro Herrera wrote: > Excerpts from Tom Lane's message of mar mar 15 11:42:06 -0300 2011: > > "Kevin Grittner" writes: > > > Tom Lane wrote: > > >> Dimitri Fontaine 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 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 pg_timezone_names provides a list of time zone names that are recognized by SET TIMEZONE, along with their associated abbreviations, UTC offsets, !and daylight-savings status. Unlike the abbreviations shown in pg_timezone_abbrevs, many of these names imply a set of daylight-savings transition date rules. Therefore, the associated information changes across local DST --- 8341,8349 The view pg_timezone_names provides a list of time zone names that are recognized by SET TIMEZONE, along with their associated abbreviations, UTC offsets, !and daylight-savings status. (Technically, !PostgreSQL uses UT1 rather !than UTC because leap seconds are not handled.) Unlike the abbreviations shown in pg_timezone_abbrevs, many of these names imply a set 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 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. --- 6898,6906 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, ! PostgreSQL uses UT1 because ! leap seconds are not handled.) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Macros for time magic values
Excerpts from Tom Lane's message of mar mar 15 11:42:06 -0300 2011: > "Kevin Grittner" writes: > > Tom Lane wrote: > >> Dimitri Fontaine 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 The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Macros for time magic values
"Kevin Grittner" writes: > Tom Lane wrote: >> Dimitri Fontaine 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Macros for time magic values
Tom Lane wrote: > Dimitri Fontaine 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Macros for time magic values
Dimitri Fontaine 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Macros for time magic values
Simon Riggs 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Macros for time magic values
Bruce Momjian 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Macros for time magic values
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 http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Macros for time magic values
Robert Haas wrote: > On Mon, Mar 14, 2011 at 12:01 PM, Tom Lane wrote: > > "Kevin Grittner" 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 http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Macros for time magic values
On Mon, Mar 14, 2011 at 12:01 PM, Tom Lane wrote: > "Kevin Grittner" 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Macros for time magic values
"Kevin Grittner" 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Macros for time magic values
Bruce Momjian 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Macros for time magic values
Tom Lane wrote: > Robert Haas writes: > > On Mon, Mar 14, 2011 at 4:22 AM, Simon Riggs 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 http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Macros for time magic values
Robert Haas writes: > On Mon, Mar 14, 2011 at 4:22 AM, Simon Riggs 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Macros for time magic values
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Macros for time magic values
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 http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Macros for time magic values
Robert Haas wrote: > On Mon, Mar 14, 2011 at 4:22 AM, 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. > > 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 http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Macros for time magic values
On 03/14/2011 09:25 AM, Robert Haas wrote: On Mon, Mar 14, 2011 at 4:22 AM, 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. 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Macros for time magic values
On Mon, Mar 14, 2011 at 4:22 AM, 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. 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Macros for time magic values
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Macros for time magic values
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Macros for time magic values
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 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_HO
Re: [HACKERS] Macros for time magic values
Bruce Momjian wrote: > Christopher Browne wrote: > > On Fri, Mar 11, 2011 at 12:50 PM, 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. > > > > 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 http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Macros for time magic values
Christopher Browne wrote: > On Fri, Mar 11, 2011 at 12:50 PM, 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. > > 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 http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Macros for time magic values
On Fri, Mar 11, 2011 at 12:50 PM, 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. The context diffs show off some references to 1901 and 2038... Here's a *possible* extension to this... -- http://linuxfinances.info/info/linuxdistributions.html diff --git a/src/backend/utils/adt/nabstime.c b/src/backend/utils/adt/nabstime.c index 0e25c5f..3cf4166 100644 --- a/src/backend/utils/adt/nabstime.c +++ b/src/backend/utils/adt/nabstime.c @@ -178,7 +178,7 @@ tm2abstime(struct pg_tm * tm, int tz) AbsoluteTime sec; /* validate, before going out of range on some members */ - if (tm->tm_year < 1901 || tm->tm_year > 2038 || + if (tm->tm_year < UTIME_MINYEAR || tm->tm_year > UTIME_MAXYEAR || tm->tm_mon < 1 || tm->tm_mon > 12 || tm->tm_mday < 1 || tm->tm_mday > 31 || tm->tm_hour < 0 || -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] 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 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(newtb