Thread: Re: [GENERAL] ISO week dates
I am seeing buildfarm failures from the new regression tests added by
this patch. Would someone research why this is happening?
http://www.pgbuildfarm.org/cgi-bin/show_status.pl
---------------------------------------------------------------------------
bruce wrote:
>
> Patch applied. Thanks.
>
> ---------------------------------------------------------------------------
>
>
> Brendan Jurd wrote:
> > The attached patch implements my proposal to extend support for the
> > ISO week date calendar.
> >
> > I have added two new format fields for use with to_char, to_date and
> > to_timestamp:
> > - ID for day-of-week
> > - IDDD for day-of-year
> >
> > This makes it possible to convert ISO week dates to and from text
> > fully represented in either week ('IYYY-IW-ID') or day-of-year
> > ('IYYY-IDDD') format.
> >
> > I have also added an 'isoyear' field for use with extract / date_part.
> >
> > The patch includes documentation updates and some extra tests in the
> > regression suite for the new fields.
> >
> > I have tried to implement these features with as little disruption to
> > the existing code as possible. I built on the existing date2iso*
> > functions in src/backend/utils/adt/timestamp.c, and added a few
> > functions of my own, but I wonder if these functions would be more
> > appropriately located in datetime.c, alongside date2j and j2date?
> >
> > I'd also like to raise the topic of how conversion from text to ISO
> > week dates should be handled, where the user has specified a bogus
> > mixture of fields. Existing code basically ignores these issues; for
> > example, if a user were to call to_date('1998-01-01 2454050',
> > 'YYYY-MM-DD J') the function returns 2006-01-01, a result of setting
> > the year field from YYYY, then overwriting year, month and day with
> > the values from the Julian date in J, then setting the month and day
> > normally from MM and DD.
> >
> > 2006-01-01 is not a valid representation of either of the values the
> > user specified. Now you might say "ask a silly question, get a silly
> > answer"; the user shouldn't send nonsense arguments to to_date and
> > expect a sensible result. But perhaps the right way to respond to a
> > broken timestamp definition is to throw an error, rather than behave
> > as though everything has gone to plan, and return something which is
> > not correct.
> >
> > The same situation can arise if the user mixes ISO and Gregorian data;
> > how should Postgres deal with something like to_date('2006-250',
> > 'IYYY-DDD')? The current behaviour in my patch is actually to assume
> > that the user meant to say 'IYYY-IDDD', since "the 250th Gregorian day
> > of the ISO year 2006" is total gibberish. But perhaps it should be
> > throwing an error message.
> >
> > That's all for now, thanks for your time.
> > BJ
>
> [ Attachment, skipping... ]
>
> >
> > ---------------------------(end of broadcast)---------------------------
> > TIP 2: Don't 'kill -9' the postmaster
>
> --
> Bruce Momjian <bruce@momjian.us> http://momjian.us
> EnterpriseDB http://www.enterprisedb.com
>
> + If your life is a hard drive, Christ can be your backup. +
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Followup --- something weird is going on. I am seeing _random_ failures of the regression tests here in that same place, and the build farm seems to fail in the same place, but with different row counts. I am heading to bed but when I wake up, if it still an issue, I will revert the patch. --------------------------------------------------------------------------- Bruce Momjian wrote: > > I am seeing buildfarm failures from the new regression tests added by > this patch. Would someone research why this is happening? > > http://www.pgbuildfarm.org/cgi-bin/show_status.pl > > --------------------------------------------------------------------------- > > bruce wrote: > > > > Patch applied. Thanks. > > > > --------------------------------------------------------------------------- > > > > > > Brendan Jurd wrote: > > > The attached patch implements my proposal to extend support for the > > > ISO week date calendar. > > > > > > I have added two new format fields for use with to_char, to_date and > > > to_timestamp: > > > - ID for day-of-week > > > - IDDD for day-of-year > > > > > > This makes it possible to convert ISO week dates to and from text > > > fully represented in either week ('IYYY-IW-ID') or day-of-year > > > ('IYYY-IDDD') format. > > > > > > I have also added an 'isoyear' field for use with extract / date_part. > > > > > > The patch includes documentation updates and some extra tests in the > > > regression suite for the new fields. > > > > > > I have tried to implement these features with as little disruption to > > > the existing code as possible. I built on the existing date2iso* > > > functions in src/backend/utils/adt/timestamp.c, and added a few > > > functions of my own, but I wonder if these functions would be more > > > appropriately located in datetime.c, alongside date2j and j2date? > > > > > > I'd also like to raise the topic of how conversion from text to ISO > > > week dates should be handled, where the user has specified a bogus > > > mixture of fields. Existing code basically ignores these issues; for > > > example, if a user were to call to_date('1998-01-01 2454050', > > > 'YYYY-MM-DD J') the function returns 2006-01-01, a result of setting > > > the year field from YYYY, then overwriting year, month and day with > > > the values from the Julian date in J, then setting the month and day > > > normally from MM and DD. > > > > > > 2006-01-01 is not a valid representation of either of the values the > > > user specified. Now you might say "ask a silly question, get a silly > > > answer"; the user shouldn't send nonsense arguments to to_date and > > > expect a sensible result. But perhaps the right way to respond to a > > > broken timestamp definition is to throw an error, rather than behave > > > as though everything has gone to plan, and return something which is > > > not correct. > > > > > > The same situation can arise if the user mixes ISO and Gregorian data; > > > how should Postgres deal with something like to_date('2006-250', > > > 'IYYY-DDD')? The current behaviour in my patch is actually to assume > > > that the user meant to say 'IYYY-IDDD', since "the 250th Gregorian day > > > of the ISO year 2006" is total gibberish. But perhaps it should be > > > throwing an error message. > > > > > > That's all for now, thanks for your time. > > > BJ > > > > [ Attachment, skipping... ] > > > > > > > > ---------------------------(end of broadcast)--------------------------- > > > TIP 2: Don't 'kill -9' the postmaster > > > > -- > > Bruce Momjian <bruce@momjian.us> http://momjian.us > > EnterpriseDB http://www.enterprisedb.com > > > > + If your life is a hard drive, Christ can be your backup. + > > -- > Bruce Momjian <bruce@momjian.us> http://momjian.us > EnterpriseDB http://www.enterprisedb.com > > + If your life is a hard drive, Christ can be your backup. + -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian escribió:
>
> Followup --- something weird is going on. I am seeing _random_ failures
> of the regression tests here in that same place, and the build farm
> seems to fail in the same place, but with different row counts.
This failure is pretty interesting:
--- 724,730 ----
date_part( 'isoyear', d1) AS isoyear, date_part( 'week', d1) AS week,
date_part( 'dow', d1) AS dow
FROM TIMESTAMP_TBL WHERE d1 BETWEEN '1902-01-01' AND '2038-01-01';
! ERROR: relation "timestamp_tbl" does not exist
-- TO_CHAR()
SELECT '' AS to_char_1, to_char(d1, 'DAY Day day DY Dy dy MONTH Month month RM MON Mon mon')
FROM TIMESTAMPTZ_TBL;
http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=agouti&dt=2007-02-16%2005:15:01
How can the table fail to exist, and yet not report a problem when it
was created?
--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera wrote: > Bruce Momjian escribió: > >> Followup --- something weird is going on. I am seeing _random_ failures >> of the regression tests here in that same place, and the build farm >> seems to fail in the same place, but with different row counts. >> > > This failure is pretty interesting: > > --- 724,730 ---- > date_part( 'isoyear', d1) AS isoyear, date_part( 'week', d1) AS week, > date_part( 'dow', d1) AS dow > FROM TIMESTAMP_TBL WHERE d1 BETWEEN '1902-01-01' AND '2038-01-01'; > ! ERROR: relation "timestamp_tbl" does not exist > -- TO_CHAR() > SELECT '' AS to_char_1, to_char(d1, 'DAY Day day DY Dy dy MONTH Month month RM MON Mon mon') > FROM TIMESTAMPTZ_TBL; > > http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=agouti&dt=2007-02-16%2005:15:01 > > How can the table fail to exist, and yet not report a problem when it > was created? > > Looks to me like the timestamptz test relies on the timestamp test (for timestamp_tbl) but they are set to run in parallel, so we have a race condition. Oops! cheers andrew
Andrew Dunstan escribió: > Looks to me like the timestamptz test relies on the timestamp test (for > timestamp_tbl) but they are set to run in parallel, so we have a race > condition. Oops! Good catch :-) I'd guess the answer is to move the tests using the timestamp_tbl to the timestamp test. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera wrote: > Andrew Dunstan escribi?: > > > Looks to me like the timestamptz test relies on the timestamp test (for > > timestamp_tbl) but they are set to run in parallel, so we have a race > > condition. Oops! > > Good catch :-) > > I'd guess the answer is to move the tests using the timestamp_tbl to the > timestamp test. OK, will do. Thanks for the diagnosis. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian escribió: > Alvaro Herrera wrote: > > Andrew Dunstan escribi?: > > > > > Looks to me like the timestamptz test relies on the timestamp test (for > > > timestamp_tbl) but they are set to run in parallel, so we have a race > > > condition. Oops! > > > > Good catch :-) > > > > I'd guess the answer is to move the tests using the timestamp_tbl to the > > timestamp test. > > OK, will do. Thanks for the diagnosis. Actually I have a patch ready for it. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera wrote: > Bruce Momjian escribió: > >> Alvaro Herrera wrote: >> >>> Andrew Dunstan escribi?: >>> >>> >>>> Looks to me like the timestamptz test relies on the timestamp test (for >>>> timestamp_tbl) but they are set to run in parallel, so we have a race >>>> condition. Oops! >>>> >>> Good catch :-) >>> >>> I'd guess the answer is to move the tests using the timestamp_tbl to the >>> timestamp test. >>> >> OK, will do. Thanks for the diagnosis. >> > > Actually I have a patch ready for it. > > Well, the first question is "which table should these tests actually be using, and why?" Then they can be changed or moved as appropriate. cheers andrew
Alvaro Herrera wrote: > Bruce Momjian escribi?: > > Alvaro Herrera wrote: > > > Andrew Dunstan escribi?: > > > > > > > Looks to me like the timestamptz test relies on the timestamp test (for > > > > timestamp_tbl) but they are set to run in parallel, so we have a race > > > > condition. Oops! > > > > > > Good catch :-) > > > > > > I'd guess the answer is to move the tests using the timestamp_tbl to the > > > timestamp test. > > > > OK, will do. Thanks for the diagnosis. > > Actually I have a patch ready for it. OK, thanks, I will not do anything. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Andrew Dunstan escribió: > Alvaro Herrera wrote: > >Bruce Momjian escribió: > > > >>Alvaro Herrera wrote: > >> > >>>Andrew Dunstan escribi?: > >>> > >>> > >>>>Looks to me like the timestamptz test relies on the timestamp test (for > >>>>timestamp_tbl) but they are set to run in parallel, so we have a race > >>>>condition. Oops! > >>>> > >>>Good catch :-) > >>> > >>>I'd guess the answer is to move the tests using the timestamp_tbl to the > >>>timestamp test. > >>> > >>OK, will do. Thanks for the diagnosis. > >> > > > >Actually I have a patch ready for it. > > > > > > Well, the first question is "which table should these tests actually be > using, and why?" Then they can be changed or moved as appropriate. I just committed my patch before reading this comment. Maybe it was rushed. On the other hand, my thinking was that the submitter used the serial schedule to test, so he wouldn't catch the problem because the timestamp test comes before the timestamptz test and so there's no race condition there. My conclusion would be that he intended to use the timestamp_tbl table because that's what he did and it worked for him. However, he did change one test from using the timestamptz_tbl into using timestamp_tbl (and changed the to_char format in the process); I fixed that by putting the new format in the timestamp test, and restoring the original in timestamptz. Lesson for today would be to make sure to run both the serial and the parallel tests, several times, and that they both pass ... -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Bruce Momjian escribió:
Maybe now would be an appropriate time to discuss the open questions in
the submitting email:
> > Brendan Jurd wrote:
> > > I have tried to implement these features with as little disruption to
> > > the existing code as possible. I built on the existing date2iso*
> > > functions in src/backend/utils/adt/timestamp.c, and added a few
> > > functions of my own, but I wonder if these functions would be more
> > > appropriately located in datetime.c, alongside date2j and j2date?
> > >
> > > I'd also like to raise the topic of how conversion from text to ISO
> > > week dates should be handled, where the user has specified a bogus
> > > mixture of fields. Existing code basically ignores these issues; for
> > > example, if a user were to call to_date('1998-01-01 2454050',
> > > 'YYYY-MM-DD J') the function returns 2006-01-01, a result of setting
> > > the year field from YYYY, then overwriting year, month and day with
> > > the values from the Julian date in J, then setting the month and day
> > > normally from MM and DD.
> > >
> > > 2006-01-01 is not a valid representation of either of the values the
> > > user specified. Now you might say "ask a silly question, get a silly
> > > answer"; the user shouldn't send nonsense arguments to to_date and
> > > expect a sensible result. But perhaps the right way to respond to a
> > > broken timestamp definition is to throw an error, rather than behave
> > > as though everything has gone to plan, and return something which is
> > > not correct.
> > >
> > > The same situation can arise if the user mixes ISO and Gregorian data;
> > > how should Postgres deal with something like to_date('2006-250',
> > > 'IYYY-DDD')? The current behaviour in my patch is actually to assume
> > > that the user meant to say 'IYYY-IDDD', since "the 250th Gregorian day
> > > of the ISO year 2006" is total gibberish. But perhaps it should be
> > > throwing an error message.
My thinking is that erroneous patterns should throw an error, and not
try to second-guess the user. (IIRC this was being discussed in some
other thread not long ago).
--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera wrote:
> > > > The same situation can arise if the user mixes ISO and Gregorian data;
> > > > how should Postgres deal with something like to_date('2006-250',
> > > > 'IYYY-DDD')? The current behaviour in my patch is actually to assume
> > > > that the user meant to say 'IYYY-IDDD', since "the 250th Gregorian day
> > > > of the ISO year 2006" is total gibberish. But perhaps it should be
> > > > throwing an error message.
>
> My thinking is that erroneous patterns should throw an error, and not
> try to second-guess the user. (IIRC this was being discussed in some
> other thread not long ago).
The author is working to consistently throw an error for all invalid
patterns. Right now, the patch just tries to do its best, which is
unfortunately consistent with what the rest of the code currently does.
:-)
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Andrew Dunstan escribi�:
>> Well, the first question is "which table should these tests actually be
>> using, and why?" Then they can be changed or moved as appropriate.
> I just committed my patch before reading this comment. Maybe it was
> rushed.
Yes, it was: you now have two duplicate tests in timestamp.sql, and
no corresponding test in timestamptz.sql. It looks to me like the
submitter intended to be testing timestamp_tbl in the former file
and the same tests against timestamptz_tbl in the latter. Please
recheck the diff and fix.
regards, tom lane
On 2/17/07, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Yes, it was: you now have two duplicate tests in timestamp.sql, and > no corresponding test in timestamptz.sql. It looks to me like the > submitter intended to be testing timestamp_tbl in the former file > and the same tests against timestamptz_tbl in the latter. Please > recheck the diff and fix. > Tom's right. Looking back at the patch, it was a simple coding error. The test in timestamptz.sql should have been querying TIMESTAMPTZ_TBL not TIMESTAMP_TBL. The intent was to run the same tests against both timestamp and timestamptz. Looks like I failed to run the parallel tests after my last round of modifications, as Alvaro suggested. That was my bad. I'll have a patch for proper handling of invalid formatting codes soon. And I'll be sure to test it in every way I can find to do so. BJ
Brendan Jurd wrote: > On 2/17/07, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Yes, it was: you now have two duplicate tests in timestamp.sql, and > > no corresponding test in timestamptz.sql. It looks to me like the > > submitter intended to be testing timestamp_tbl in the former file > > and the same tests against timestamptz_tbl in the latter. Please > > recheck the diff and fix. > > > > Tom's right. Looking back at the patch, it was a simple coding error. > The test in timestamptz.sql should have been querying TIMESTAMPTZ_TBL > not TIMESTAMP_TBL. The intent was to run the same tests against both > timestamp and timestamptz. > > Looks like I failed to run the parallel tests after my last round of > modifications, as Alvaro suggested. That was my bad. > > I'll have a patch for proper handling of invalid formatting codes > soon. And I'll be sure to test it in every way I can find to do so. Great. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Brendan Jurd escribió: > On 2/17/07, Tom Lane <tgl@sss.pgh.pa.us> wrote: > >Yes, it was: you now have two duplicate tests in timestamp.sql, and > >no corresponding test in timestamptz.sql. It looks to me like the > >submitter intended to be testing timestamp_tbl in the former file > >and the same tests against timestamptz_tbl in the latter. Please > >recheck the diff and fix. > > > > Tom's right. Looking back at the patch, it was a simple coding error. > The test in timestamptz.sql should have been querying TIMESTAMPTZ_TBL > not TIMESTAMP_TBL. The intent was to run the same tests against both > timestamp and timestamptz. Thanks for the clarification. Would you have a look at the tests as they are now and confirm that that's what you wanted? > > I'll have a patch for proper handling of invalid formatting codes > soon. And I'll be sure to test it in every way I can find to do so. Great. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On 2/17/07, Alvaro Herrera <alvherre@commandprompt.com> wrote: > Thanks for the clarification. Would you have a look at the tests as > they are now and confirm that that's what you wanted? > Yes, the tests in HEAD right now are what I wanted. Although, while I was in there I did notice a minor thing (another copy-paste error): the timestamptz test is using a column alias that's not consistent with other tests in the vicinity. Patch attached.
Attachment
Applied. --------------------------------------------------------------------------- Brendan Jurd wrote: > On 2/17/07, Alvaro Herrera <alvherre@commandprompt.com> wrote: > > Thanks for the clarification. Would you have a look at the tests as > > they are now and confirm that that's what you wanted? > > > > Yes, the tests in HEAD right now are what I wanted. Although, while I > was in there I did notice a minor thing (another copy-paste error): > the timestamptz test is using a column alias that's not consistent > with other tests in the vicinity. Patch attached. [ Attachment, skipping... ] -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +