Re: oh dear ...

This worked in 7.3:

regression=# select '1999-jan-08'::date;
ERROR: date/time field value out of range: "1999-jan-08"
HINT: Perhaps you need a different "datestyle" setting.

Setting DateStyle to YMD doesn't help, and in any case I'd think that
this ought to be considered an unambiguous input format.

The variants
select 'jan-08-1999'::date;
select '08-jan-1999'::date;
both still work, so I think this is probably some small oversight in the
logic, but I haven't dug into it to find where.

Not sure if this qualifies as a must-fix-for-7.4 or not, but my vote
would be "yes" ...

regards, tom lane

I said:
> This worked in 7.3:
> regression=# select '1999-jan-08'::date;
> ERROR: date/time field value out of range: "1999-jan-08"
> HINT: Perhaps you need a different "datestyle" setting.

> Setting DateStyle to YMD doesn't help, and in any case I'd think that
> this ought to be considered an unambiguous input format.

This appears to be an oversight in the portions of the datetime code
that we recently changed to enforce DateStyle more tightly.
Specifically, DecodeNumber was rewritten without realizing that it was
invoked in a special way when a textual month name appears in the input.
DecodeDate actually makes two passes over the input, noting the textual
month name in the first pass, and then calling DecodeNumber on only the
numeric fields in the second pass. This means that when DecodeNumber is
called for the first time, the MONTH flag may already be set. The
rewrite mistakenly assumed that in this case we must be at the second
field of an MM-DD-YY-order input.

I propose the attached patch to fix the problem. It doesn't break any
regression tests, and it appears to fix the cases noted in its comment.

Opinions on whether to apply this to 7.4?

regards, tom lane

On Fri, 14 Nov 2003, Tom Lane wrote:

> I said:
> > This worked in 7.3:
> > regression=# select '1999-jan-08'::date;
> > ERROR: date/time field value out of range: "1999-jan-08"
> > HINT: Perhaps you need a different "datestyle" setting.
>
> > Setting DateStyle to YMD doesn't help, and in any case I'd think that
> > this ought to be considered an unambiguous input format.
>
> This appears to be an oversight in the portions of the datetime code
> that we recently changed to enforce DateStyle more tightly.
> Specifically, DecodeNumber was rewritten without realizing that it was
> invoked in a special way when a textual month name appears in the input.
> DecodeDate actually makes two passes over the input, noting the textual
> month name in the first pass, and then calling DecodeNumber on only the
> numeric fields in the second pass. This means that when DecodeNumber is
> called for the first time, the MONTH flag may already be set. The
> rewrite mistakenly assumed that in this case we must be at the second
> field of an MM-DD-YY-order input.
>
> I propose the attached patch to fix the problem. It doesn't break any
> regression tests, and it appears to fix the cases noted in its comment.
>
> Opinions on whether to apply this to 7.4?

based on "ought to be considered an unambiguous input format", I'd say
leave it for 7.4.1 ...

Tom Lane wrote:
> I said:
> > This worked in 7.3:
> > regression=# select '1999-jan-08'::date;
> > ERROR: date/time field value out of range: "1999-jan-08"
> > HINT: Perhaps you need a different "datestyle" setting.
>
> > Setting DateStyle to YMD doesn't help, and in any case I'd think that
> > this ought to be considered an unambiguous input format.
>
> This appears to be an oversight in the portions of the datetime code
> that we recently changed to enforce DateStyle more tightly.
> Specifically, DecodeNumber was rewritten without realizing that it was
> invoked in a special way when a textual month name appears in the input.
> DecodeDate actually makes two passes over the input, noting the textual
> month name in the first pass, and then calling DecodeNumber on only the
> numeric fields in the second pass. This means that when DecodeNumber is
> called for the first time, the MONTH flag may already be set. The
> rewrite mistakenly assumed that in this case we must be at the second
> field of an MM-DD-YY-order input.
>
> I propose the attached patch to fix the problem. It doesn't break any
> regression tests, and it appears to fix the cases noted in its comment.
>
> Opinions on whether to apply this to 7.4?

I guess the question is whether we would fix this in a minor release,
and I think the answer it yes, so we can fix it now.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

On Fri, 14 Nov 2003, Bruce Momjian wrote:

> Tom Lane wrote:
> > I said:
> > > This worked in 7.3:
> > > regression=# select '1999-jan-08'::date;
> > > ERROR: date/time field value out of range: "1999-jan-08"
> > > HINT: Perhaps you need a different "datestyle" setting.
> >
> > > Setting DateStyle to YMD doesn't help, and in any case I'd think that
> > > this ought to be considered an unambiguous input format.
> >
> > This appears to be an oversight in the portions of the datetime code
> > that we recently changed to enforce DateStyle more tightly.
> > Specifically, DecodeNumber was rewritten without realizing that it was
> > invoked in a special way when a textual month name appears in the input.
> > DecodeDate actually makes two passes over the input, noting the textual
> > month name in the first pass, and then calling DecodeNumber on only the
> > numeric fields in the second pass. This means that when DecodeNumber is
> > called for the first time, the MONTH flag may already be set. The
> > rewrite mistakenly assumed that in this case we must be at the second
> > field of an MM-DD-YY-order input.
> >
> > I propose the attached patch to fix the problem. It doesn't break any
> > regression tests, and it appears to fix the cases noted in its comment.
> >
> > Opinions on whether to apply this to 7.4?
>
> I guess the question is whether we would fix this in a minor release,
> and I think the answer it yes, so we can fix it now.

Ah, so we attempt to fix a bug that affects what appears to be a small %
of configurations with "quick testing" and with the greater possibility of
affecting a larger % of configurations ... instead of releasing what we
has been reported as being stable on the large % of configurations, and
fixing it for that small % of configuratiosn in a minor release?

Sounds to me like a decision design to benefit the few at the risk of the
many ... when documenting the known bug for those few would be safer ...

Marc G. Fournier wrote:

>On Fri, 14 Nov 2003, Bruce Momjian wrote:
>
>
>
>>Tom Lane wrote:
>>
>>
>>>I said:
>>>
>>>
>>>>This worked in 7.3:
>>>>regression=# select '1999-jan-08'::date;
>>>>ERROR: date/time field value out of range: "1999-jan-08"
>>>>HINT: Perhaps you need a different "datestyle" setting.
>>>>
>>>>
>>>>Setting DateStyle to YMD doesn't help, and in any case I'd think that
>>>>this ought to be considered an unambiguous input format.
>>>>
>>>>
>>>This appears to be an oversight in the portions of the datetime code
>>>that we recently changed to enforce DateStyle more tightly.
>>>Specifically, DecodeNumber was rewritten without realizing that it was
>>>invoked in a special way when a textual month name appears in the input.
>>>DecodeDate actually makes two passes over the input, noting the textual
>>>month name in the first pass, and then calling DecodeNumber on only the
>>>numeric fields in the second pass. This means that when DecodeNumber is
>>>called for the first time, the MONTH flag may already be set. The
>>>rewrite mistakenly assumed that in this case we must be at the second
>>>field of an MM-DD-YY-order input.
>>>
>>>I propose the attached patch to fix the problem. It doesn't break any
>>>regression tests, and it appears to fix the cases noted in its comment.
>>>
>>>Opinions on whether to apply this to 7.4?
>>>
>>>
>>I guess the question is whether we would fix this in a minor release,
>>and I think the answer it yes, so we can fix it now.
>>
>>
>
>Ah, so we attempt to fix a bug that affects what appears to be a small %
>of configurations with "quick testing" and with the greater possibility of
>affecting a larger % of configurations ... instead of releasing what we
>has been reported as being stable on the large % of configurations, and
>fixing it for that small % of configuratiosn in a minor release?
>
>Sounds to me like a decision design to benefit the few at the risk of the
>many ... when documenting the known bug for those few would be safer ...
>
>
>

I'm confused. My understanding from what Tom said is that it affects all
configurations.

cheers

andrew

On Fri, 14 Nov 2003, Andrew Dunstan wrote:

> I'm confused. My understanding from what Tom said is that it affects all
> configurations.

the stats collector problem, from what I've seen through this list,
affects Solaris, and only some Solaris configuration ..

"Marc G. Fournier" <scrappy(at)postgresql(dot)org> writes:
> On Fri, 14 Nov 2003, Bruce Momjian wrote:
>> I guess the question is whether we would fix this in a minor release,
>> and I think the answer it yes, so we can fix it now.

> Ah, so we attempt to fix a bug that affects what appears to be a small %
> of configurations with "quick testing" and with the greater possibility of
> affecting a larger % of configurations ... instead of releasing what we
> has been reported as being stable on the large % of configurations, and
> fixing it for that small % of configuratiosn in a minor release?

Huh? The pgstat bug is a platform dependency, sure, but this datetime
bug is not platform-specific. I don't see that there's much commonality
in the criteria for whether to patch them.

My vote is to patch both --- I don't like shipping releases with known
bugs in them, when such bugs would have been patched with no discussion
just a week earlier. For sure we should triple-check the proposed
patches, but once that's done I don't see a reason to hold off.

The pgstat patch has already been checked to my satisfaction, but the
datetime patch needs more eyeballs on it; anyone out there have time to
look at it?

regards, tom lane

Marc G. Fournier wrote:
>
> On Fri, 14 Nov 2003, Andrew Dunstan wrote:
>>I'm confused. My understanding from what Tom said is that it affects all
>>configurations.
>
> the stats collector problem, from what I've seen through this list,
> affects Solaris, and only some Solaris configuration ..
>

But the issue at hand is this one:

Tom Lane wrote:
> This worked in 7.3:
>
> regression=# select '1999-jan-08'::date;
> ERROR: date/time field value out of range: "1999-jan-08"
> HINT: Perhaps you need a different "datestyle" setting.

Seems like this would affect everyone who uses this style of date in
their app. If it isn't a must fix for 7.4, we should plan 7.4.1 for a
fairly quick follow up.

Joe

> I propose the attached patch to fix the problem. It doesn't break any
> regression tests, and it appears to fix the cases noted in its comment.
>
> Opinions on whether to apply this to 7.4?

I think it should be fixed, since it could cause applications to break.
Shouldn't you also add a regression test to catch this in the future?

Chris

Christopher Kings-Lynne <chriskl(at)familyhealth(dot)com(dot)au> writes:
> Shouldn't you also add a regression test to catch this in the future?

Yes, I absolutely plan to stick some regression test additions into HEAD.
There's not a need for such changes in the 7.4 branch though. Right at
the moment what we need is a decision about whether to apply the code
change to 7.4 release ...

regards, tom lane

Tom Lane wrote:
> The pgstat patch has already been checked to my satisfaction, but the
> datetime patch needs more eyeballs on it; anyone out there have time to
> look at it?

FWIW, it looks good to me, seems to work as intended, and passes all
existing regression tests.

Joe

My bad, confused two different issues in one thread :(

On Fri, 14 Nov 2003, Joe Conway wrote:

> Marc G. Fournier wrote:
> >
> > On Fri, 14 Nov 2003, Andrew Dunstan wrote:
> >>I'm confused. My understanding from what Tom said is that it affects all
> >>configurations.
> >
> > the stats collector problem, from what I've seen through this list,
> > affects Solaris, and only some Solaris configuration ..
> >
>
> But the issue at hand is this one:
>
> Tom Lane wrote:
> > This worked in 7.3:
> >
> > regression=# select '1999-jan-08'::date;
> > ERROR: date/time field value out of range: "1999-jan-08"
> > HINT: Perhaps you need a different "datestyle" setting.
>
> Seems like this would affect everyone who uses this style of date in
> their app. If it isn't a must fix for 7.4, we should plan 7.4.1 for a
> fairly quick follow up.
>
> Joe
>
>

On Fri, 14 Nov 2003, Tom Lane wrote:

> Christopher Kings-Lynne <chriskl(at)familyhealth(dot)com(dot)au> writes:
> > Shouldn't you also add a regression test to catch this in the future?
>
> Yes, I absolutely plan to stick some regression test additions into HEAD.
> There's not a need for such changes in the 7.4 branch though. Right at
> the moment what we need is a decision about whether to apply the code
> change to 7.4 release ...

Go for it ...

Joe Conway <mail(at)joeconway(dot)com> writes:
> Tom Lane wrote:
>> The pgstat patch has already been checked to my satisfaction, but the
>> datetime patch needs more eyeballs on it; anyone out there have time to
>> look at it?

> FWIW, it looks good to me, seems to work as intended, and passes all
> existing regression tests.

I made up a more thorough regression test for date input formats, and
found that there were still some cases that were rejected :-(. Attached
is a more complete patch that handles all month-name cases, and
explicitly can not change the behavior when there's not a textual month
name. Documentation addition and regression test included.

I'd like some further review of this before I risk applying it to 7.4
though ... anyone have time today?

regards, tom lane

> I made up a more thorough regression test for date input formats, and
> found that there were still some cases that were rejected :-(. Attached
> is a more complete patch that handles all month-name cases, and
> explicitly can not change the behavior when there's not a textual month
> name. Documentation addition and regression test included.
>
> I'd like some further review of this before I risk applying it to 7.4
> though ... anyone have time today?

Hi Tom,

Everything passes for me on FreeBSD 4.8, latest CVS, "gmake check", with
your patch applied.

Chris

Christopher Kings-Lynne wrote:
>> I made up a more thorough regression test for date input formats, and
>> found that there were still some cases that were rejected :-(. Attached
>> is a more complete patch that handles all month-name cases, and
>> explicitly can not change the behavior when there's not a textual month
>> name. Documentation addition and regression test included.
>>
>> I'd like some further review of this before I risk applying it to 7.4
>> though ... anyone have time today?
>
> Everything passes for me on FreeBSD 4.8, latest CVS, "gmake check", with
> your patch applied.

Same here on RH 9.

Joe