Обсуждение: Strange behavior with leap dates and centuries BC

I saw this strange behavior due to a customer problem who managed to get 
dumps which aren't restorable anymore:

CREATE TABLE foo(datum date);

INSERT INTO foo VALUES('0000-02-29');
INSERT 0 1

SELECT * FROM foo;    datum
---------------0001-02-29 BC
(1 row)

COPY foo TO STDOUT;
0001-02-29 BC

INSERT INTO foo VALUES('0001-02-29 BC');
ERROR:  date/time field value out of range: "0001-02-29 BC"


Huh? It seems the calculation for leap dates with negative year values is 
broken. This example was taken from a current HEAD checkout today, the 
original version i've seen this behavior first was 8.2.4.



--  Thanks
                   Bernd

Bernd Helmle <mailings@oopsware.de> writes:
> CREATE TABLE foo(datum date);
> INSERT INTO foo VALUES('0000-02-29');

Since there is no year zero according to Gregorian reckoning, this
should have been rejected to start with.

> INSERT INTO foo VALUES('0001-02-29 BC');
> ERROR:  date/time field value out of range: "0001-02-29 BC"

Yeah, something broken there too.  It does know (correctly) that 1BC
is a leap year:

regression=# select '0001-02-28 BC'::date + 1;  ?column?    
---------------0001-02-29 BC
(1 row)

regression=# select '0002-02-28 BC'::date + 1;  ?column?    
---------------0002-03-01 BC
(1 row)

So I'd say there are two separate bugs in datetime input processing
exposed here.

> Huh? It seems the calculation for leap dates with negative year values is 
> broken. This example was taken from a current HEAD checkout today, the 
> original version i've seen this behavior first was 8.2.4.

I see the same behaviors in 7.4.x, so it's a longstanding problem...
        regards, tom lane

--On Montag, Februar 25, 2008 12:00:05 -0500 Tom Lane <tgl@sss.pgh.pa.us> 
wrote:

> regression=# select '0001-02-28 BC'::date + 1;
>    ?column?
> ---------------
>  0001-02-29 BC
> (1 row)
>
> regression=# select '0002-02-28 BC'::date + 1;
>    ?column?
> ---------------
>  0002-03-01 BC
> (1 row)

I stepped through the code in datetime.c and it seems the culprit here is 
DecodeDate(). It get's the date string from DecodeDateTime(), but without 
the 'BC' century notation. However, it then performs the following check
/* there is no year zero in AD/BC notation; i.e. "1 BC" == year 0 */if (bc){    if (tm->tm_year > 0)        tm->tm_year
=-(tm->tm_year - 1);    else        ereport(ERROR,                (errcode(ERRCODE_INVALID_DATETIME_FORMAT),
    errmsg("inconsistent use of year %04d and \"BC\"",                        tm->tm_year)));}
 

bc never becames true during parsing and the final check for the leap date 
fails:
/* We don't want to hint about DateStyle for Feb 29 */if (tm->tm_mday > day_tab[isleap(tm->tm_year)][tm->tm_mon - 1]){
 return DTERR_FIELD_OVERFLOW;}
 

Maybe that helps a little bit.

--  Thanks
                   Bernd

Bernd Helmle <mailings@oopsware.de> writes:
> I stepped through the code in datetime.c and it seems the culprit here is 
> DecodeDate(). It get's the date string from DecodeDateTime(), but without 
> the 'BC' century notation. However, it then performs the following check

Yeah, I had just come to the same conclusion.  It is premature for
DecodeDate to be trying to check this, because AFAICT there is no input
syntax in which it will be given both the date fields and the AD/BC
field.  There is already checking code at the bottom of DecodeDateTime,
so it looks to me like DecodeDate's checks are simply redundant in that
code path.  They aren't redundant in the calls from DecodeTimeOnly,
however.  I think we should move the date range checks and BC adjustment
into a separate function ValidateDate() that is called from the bottom
of the outer loops in DecodeDateTime/DecodeTimeOnly.

The other issue is whether to throw error for year zero, rather than
silently interpreting it as 1 BC.  I can't recall whether that behavior
was intentional at the time, but given our current rather strict
interpretation of date validity checking, it hardly seems like a good
idea now.  What I suggest is that we throw error in 8.4 and beyond,
but not back-patch that change, so as to avoid introducing a behavioral
change in minor releases.
        regards, tom lane

--On Montag, Februar 25, 2008 14:04:18 -0500 Tom Lane <tgl@sss.pgh.pa.us> 
wrote:

> The other issue is whether to throw error for year zero, rather than
> silently interpreting it as 1 BC.  I can't recall whether that behavior
> was intentional at the time, but given our current rather strict
> interpretation of date validity checking, it hardly seems like a good
> idea now.  What I suggest is that we throw error in 8.4 and beyond,
> but not back-patch that change, so as to avoid introducing a behavioral
> change in minor releases.

That sounds reasonable. I'm still trying to find out how it was managed to 
get such a date into the database, since it seems not to be intended 
behavior by the client. Maybe it's an errorneous to_date() formatting.

--  Thanks
                   Bernd

Bernd Helmle wrote:
> --On Montag, Februar 25, 2008 14:04:18 -0500 Tom Lane <tgl@sss.pgh.pa.us> 
> wrote:
> 
> > The other issue is whether to throw error for year zero, rather than
> > silently interpreting it as 1 BC.  I can't recall whether that behavior
> > was intentional at the time, but given our current rather strict
> > interpretation of date validity checking, it hardly seems like a good
> > idea now.  What I suggest is that we throw error in 8.4 and beyond,
> > but not back-patch that change, so as to avoid introducing a behavioral
> > change in minor releases.
> 
> That sounds reasonable. I'm still trying to find out how it was managed to 
> get such a date into the database, since it seems not to be intended 
> behavior by the client. Maybe it's an errorneous to_date() formatting.

Tom has applied a fix for this to CVS HEAD and back branches.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://postgres.enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +