roundoff problem in time datatype
Inserting into a time field with limited precision rounds off, which
is good except for this case:
regression=# select '23:59:59.9'::time(0);
time
----------
24:00:00
(1 row)
This is bad because:
regression=# select '24:00:00'::time(0);
ERROR: date/time field value out of range: "24:00:00"
which means that data originally accepted will fail to dump and reload.
I see this behavior in all versions back to 7.3. 7.2 was even more
broken:
regression=# select '23:59:59.9'::time(0);
time
----------
00:00:00
(1 row)
I think the correct behavior has to be to check for overflow again
after rounding off. Alternatively: why are we forbidding the value
24:00:00 anyway? Is there a reason not to allow the hours field
to exceed 23?
regards, tom lane
On Sun, 25 Sep 2005, Tom Lane wrote:
Alternatively: why are we forbidding the value 24:00:00 anyway? Is
there a reason not to allow the hours field to exceed 23?
One reason is because it's what the standard demand. Another is that it
isn't a proper time, just like feb 31 isn't a proper date.
--
/Dennis Bj�rklund
Actually, I think there is a case where 24:00 is a proper time. Isn't
it used for adding leap seconds ?
Dave
On 26-Sep-05, at 3:39 AM, Dennis Bjorklund wrote:
Show quoted text
On Sun, 25 Sep 2005, Tom Lane wrote:
Alternatively: why are we forbidding the value 24:00:00 anyway? Is
there a reason not to allow the hours field to exceed 23?One reason is because it's what the standard demand. Another is
that it
isn't a proper time, just like feb 31 isn't a proper date.--
/Dennis Björklund---------------------------(end of
broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that
your
message can get through to the mailing list cleanly
Dave Cramer <pg@fastcrypt.com> writes:
Actually, I think there is a case where 24:00 is a proper time. Isn't
it used for adding leap seconds ?
No, I think the usual notation for a leap-second is '23:59:60'.
We do allow 60 in the seconds field for this purpose.
I suppose there's another possible approach, which is to special-case
the output of this value to look like '23:59:60' instead of '24:00:00'.
Then it could be reloaded. On the whole though, most people who came
across that behavior would probably think it's a bug...
regards, tom lane
On Mon, 26 Sep 2005, Tom Lane wrote:
Actually, I think there is a case where 24:00 is a proper time. Isn't
it used for adding leap seconds ?No, I think the usual notation for a leap-second is '23:59:60'.
We do allow 60 in the seconds field for this purpose.
Yes, and it can go up to 23:59:60.999999 (depending on how many fractional
seconds one want).
I suppose there's another possible approach, which is to special-case
the output of this value to look like '23:59:60' instead of '24:00:00'.
You would get the same problem with 23:59:60.9 which I guess you want to
round up.
One "solution" is to round '23:59:59.9'::time(0) up to '00:00:00'. That is
normally the next following time value after all. I know why you might not
want to round it "up" to 00:00:00, but it's one logical solution.
By the way, here is another example of the same problem:
# SELECT time '23:59:59.9' + interval '0.1';
?column?
----------
24:00:00
# SELECT time '23:59:59.9' + interval '0.11';
?column?
-------------
00:00:00.01
(1 rad)
--
/Dennis Bj�rklund
Dennis Bjorklund <db@zigo.dhs.org> writes:
On Mon, 26 Sep 2005, Tom Lane wrote:
No, I think the usual notation for a leap-second is '23:59:60'.
We do allow 60 in the seconds field for this purpose.
Yes, and it can go up to 23:59:60.999999 (depending on how many fractional
seconds one want).
That's an urban legend. There never have been, and never will be, two
leap seconds instituted in the same minute. We really should reject
anything larger than '23:59:60'.
One "solution" is to round '23:59:59.9'::time(0) up to '00:00:00'.
7.2 did that, and we concluded it was broken.
regards, tom lane
On Mon, 26 Sep 2005, Tom Lane wrote:
Yes, and it can go up to 23:59:60.999999 (depending on how many fractional
seconds one want).That's an urban legend. There never have been, and never will be, two
leap seconds instituted in the same minute. We really should reject
anything larger than '23:59:60'.
The above is still just one leap second. The time continues to tick until
it wraps over to 00:00:00. So for example a time value of 23:59:60.42
exists if we allow just one leap second.
One "solution" is to round '23:59:59.9'::time(0) up to '00:00:00'.
7.2 did that, and we concluded it was broken.
Doesn't mean that it necissary was a correct conclusion (and I'm not
stating that it was wrong, I would like to think about it for a while
before I claim something like that).
Do the sql standard say anything on the matter?
--
/Dennis Bj�rklund
Tom Lane <tgl@sss.pgh.pa.us> writes:
That's an urban legend. There never have been, and never will be, two
leap seconds instituted in the same minute. We really should reject
anything larger than '23:59:60'.
I don't understand. The last second of a normal minute goes from 59.0 to
59.999 (etc) before the next minute begins. So surely the last second of a
minute containing a leap second goes from 60.0 to 60.999?
--
greg
On 9/26/05, Dennis Bjorklund wrote:
On Sun, 25 Sep 2005, Tom Lane wrote:
Alternatively: why are we forbidding the value 24:00:00 anyway? Is
there a reason not to allow the hours field to exceed 23?One reason is because it's what the standard demand.
Could you cite that? The only thing I can find in the SQL standard is
that the hour field in an INTERVAL can not exceed 23, not datetimes.
Another is that it
isn't a proper time, just like feb 31 isn't a proper date.
IIRC ISO 8601 (to whcih the SQL standard points) says
2005-10-01T24:00:00 is valid (and happens to be the same as
2005-10-02T00:00:00). It does seem a bit inconsistent with the spec of
an interval though.
Jochem
Jochem van Dieten <jochemd@gmail.com> writes:
On 9/26/05, Dennis Bjorklund wrote:
One reason is because it's what the standard demand.
Could you cite that? The only thing I can find in the SQL standard is
that the hour field in an INTERVAL can not exceed 23, not datetimes.
SQL99 has
_____________Table_11-Valid_values_for_datetime_fields_____________
_Keyword____________Valid_values_of_datetime_fields________________
| YEAR | 0001 to 9999 |
| | |
| MONTH | 01 to 12 |
| | |
| DAY | Within the range 1 (one) to 31, but further |
constrained by the value of MONTH and YEAR
fields, according to the rules for well-
formed dates in the Gregorian calendar.
| HOUR | 00 to 23 |
| | |
| MINUTE | 00 to 59 |
| | |
| SECOND | 00 to 61.9(N) where "9(N)" indicates |
the number of digits specified by <time
fractional seconds precision>.
| TIMEZONE_HOUR | -12 to 13 |
| | |
|_TIMEZONE_MINUTE__|_-59_to_59_____________________________________|
| | |
NOTE 62 - Datetime data types will allow dates in the Gregorian
format to be stored in the date range 0001-01-01 CE through
9999-12-31 CE. The range for SECOND allows for as many as two
"leap seconds". Interval arithmetic that involves leap seconds
or discontinuities in calendars will produce implementation-
defined results.
The urban legend about needing 2 leap seconds in the same minute has
infected the standard I see. It should only allow 60.9999 as the max
value for SECOND.
Note however that we feel free to exceed the spec in other aspects of
this --- we exceed their year range for instance. So I don't think we
necessarily have to reject '24:00:00'.
Also, the spec explicitly states that arithmetic on TIME values is done
modulo 24 hours. So it's correct for '23:59:59'::time + '1 second'::interval
to yield '00:00:00', but this does not necessarily mean that we should
cause rounding to behave that way. Depends whether you think that
rounding is an arithmetic operation or not ...
regards, tom lane
Dennis Bjorklund <db@zigo.dhs.org> writes:
Do the sql standard say anything on the matter?
It doesn't seem very helpful. AFAICS, we should interpret storing
'23:59:59.99' into a TIME(0) field as a cast from TIME(2) to TIME(0),
and the spec defines that as
15) If TD is the datetime data type TIME WITHOUT TIME ZONE, then let
TSP be the <time precision> of TD.
b) If SD is TIME WITHOUT TIME ZONE, then TV is SV, with
implementation-defined rounding or truncation if necessary.
So it's "implementation-defined" what we do.
regards, tom lane
Tom Lane wrote:
Dennis Bjorklund <db@zigo.dhs.org> writes:
On Mon, 26 Sep 2005, Tom Lane wrote:
No, I think the usual notation for a leap-second is '23:59:60'.
We do allow 60 in the seconds field for this purpose.Yes, and it can go up to 23:59:60.999999 (depending on how many fractional
seconds one want).That's an urban legend. There never have been, and never will be, two
leap seconds instituted in the same minute. We really should reject
anything larger than '23:59:60'.
mmm. The second "60" have is on duration of 1 second so 23:59:60.4 have
is right to exist.
Regards
Gaetano Mendola
On Mon, 26 Sep 2005, Tom Lane wrote:
b) If SD is TIME WITHOUT TIME ZONE, then TV is SV, with
implementation-defined rounding or truncation if necessary.So it's "implementation-defined" what we do.
Truncation would avoid the problem but of course loses some of the info.
So, what are the alternatives:
* Truncation.
* Rounding and let it wrap when rounding up towards midnight.
* Rounding and never let it wrap. The cases that would wrap
goes to 23:59:59 (or 23:59:59.9 and so on for other precisions)
or to 23:59:60 (or 23:59.60.9 and so on) if one start with a
leap second time.
Are there any more viable cases?
--
/Dennis Bj�rklund
Tom Lane wrote:
Dennis Bjorklund <db@zigo.dhs.org> writes:
Do the sql standard say anything on the matter?
It doesn't seem very helpful. AFAICS, we should interpret storing
'23:59:59.99' into a TIME(0) field as a cast from TIME(2) to TIME(0),
and the spec defines that as15) If TD is the datetime data type TIME WITHOUT TIME ZONE, then let
TSP be the <time precision> of TD.b) If SD is TIME WITHOUT TIME ZONE, then TV is SV, with
implementation-defined rounding or truncation if necessary.So it's "implementation-defined" what we do.
IMHO Since 23:59:59.99 probably means "the last milliseconds of this
day, as far as precision allows to express it", this should be truncated
to 23:59:59, not rounded to 24:00:00. Until the last microsecond has
elapsed, it's not 24 hours (you wouldn't round "happy new year" at
23:59:30 from a clock with minutes only either)
Regards,
Andreas
Andreas Pflug <pgadmin@pse-consulting.de> writes:
Tom Lane wrote:
b) If SD is TIME WITHOUT TIME ZONE, then TV is SV, with
implementation-defined rounding or truncation if necessary.So it's "implementation-defined" what we do.
IMHO Since 23:59:59.99 probably means "the last milliseconds of this
day, as far as precision allows to express it", this should be truncated
to 23:59:59, not rounded to 24:00:00. Until the last microsecond has
elapsed, it's not 24 hours (you wouldn't round "happy new year" at
23:59:30 from a clock with minutes only either)
Hm, so the proposal is "round unless that would produce 24:00:00, in
which case truncate"? Seems a bit ugly but it would follow the letter
of the spec, and avoid rejecting inputs that we used to accept. It's
still not very clear what to do with '23:59:60.9' though.
regards, tom lane
Tom Lane wrote:
Hm, so the proposal is "round unless that would produce 24:00:00, in
which case truncate"? Seems a bit ugly but it would follow the letter
of the spec, and avoid rejecting inputs that we used to accept. It's
still not very clear what to do with '23:59:60.9' though.
I'd handle it the same; 23.59.60.9 -> 23.59.60 since this is apparently
a leap second. A normal second should never become a leap second from
some conversion, but a leap second should stay one.
Regards,
Andreas
On Mon, Sep 26, 2005 at 06:23:06PM +0200, Andreas Pflug wrote:
Tom Lane wrote:
Dennis Bjorklund <db@zigo.dhs.org> writes:
Do the sql standard say anything on the matter?
It doesn't seem very helpful. AFAICS, we should interpret storing
'23:59:59.99' into a TIME(0) field as a cast from TIME(2) to TIME(0),
and the spec defines that as15) If TD is the datetime data type TIME WITHOUT TIME ZONE, then
let
TSP be the <time precision> of TD.b) If SD is TIME WITHOUT TIME ZONE, then TV is SV, with
implementation-defined rounding or truncation if necessary.So it's "implementation-defined" what we do.
IMHO Since 23:59:59.99 probably means "the last milliseconds of this
day, as far as precision allows to express it", this should be truncated
to 23:59:59, not rounded to 24:00:00. Until the last microsecond has
elapsed, it's not 24 hours (you wouldn't round "happy new year" at
23:59:30 from a clock with minutes only either)
Maybe also allow for a warning to be generated? Or some way to signal an
overflow?
I think it could be valid to do this, or round up to 24:00:00 or 'round
up' to 00:00:00, depending on what the app was trying to accomplish.
Would it be possible to allow an option to the datatype that specifies
the rounding behavior, or would they need to be different datatypes?
--
Jim C. Nasby, Sr. Engineering Consultant jnasby@pervasive.com
Pervasive Software http://pervasive.com work: 512-231-6117
vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461
On Mon, Sep 26, 2005 at 11:46:47AM -0400, Tom Lane wrote:
Jochem van Dieten <jochemd@gmail.com> writes:
On 9/26/05, Dennis Bjorklund wrote:
One reason is because it's what the standard demand.
Could you cite that? The only thing I can find in the SQL standard is
that the hour field in an INTERVAL can not exceed 23, not datetimes.SQL99 has
_____________Table_11-Valid_values_for_datetime_fields_____________
_Keyword____________Valid_values_of_datetime_fields________________
| YEAR | 0001 to 9999 |
| | |
| MONTH | 01 to 12 |
| | |
| DAY | Within the range 1 (one) to 31, but further |
constrained by the value of MONTH and YEAR
fields, according to the rules for well-
formed dates in the Gregorian calendar.| HOUR | 00 to 23 |
| | |
| MINUTE | 00 to 59 |
| | |
| SECOND | 00 to 61.9(N) where "9(N)" indicates |
the number of digits specified by <time
fractional seconds precision>.| TIMEZONE_HOUR | -12 to 13 |
| | |
|_TIMEZONE_MINUTE__|_-59_to_59_____________________________________|
| | |
NOTE 62 - Datetime data types will allow dates in the Gregorian
format to be stored in the date range 0001-01-01 CE through
9999-12-31 CE. The range for SECOND allows for as many as two
"leap seconds". Interval arithmetic that involves leap seconds
or discontinuities in calendars will produce implementation-
defined results.The urban legend about needing 2 leap seconds in the same minute has
infected the standard I see. It should only allow 60.9999 as the max
value for SECOND.Note however that we feel free to exceed the spec in other aspects of
this --- we exceed their year range for instance. So I don't think we
necessarily have to reject '24:00:00'.Also, the spec explicitly states that arithmetic on TIME values is done
modulo 24 hours. So it's correct for '23:59:59'::time + '1 second'::interval
to yield '00:00:00', but this does not necessarily mean that we should
cause rounding to behave that way. Depends whether you think that
rounding is an arithmetic operation or not ...
Does that portion of the spec also apply to plain time fields? The
entire issue here only exists because there's no method to handle the
overflow, unlike in a timestamp.
--
Jim C. Nasby, Sr. Engineering Consultant jnasby@pervasive.com
Pervasive Software http://pervasive.com work: 512-231-6117
vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461
Where are we on this? I see current CVS behaving the same as below,
except the last query now returns 24:00:00.
---------------------------------------------------------------------------
Tom Lane wrote:
Inserting into a time field with limited precision rounds off, which
is good except for this case:regression=# select '23:59:59.9'::time(0);
time
----------
24:00:00
(1 row)This is bad because:
regression=# select '24:00:00'::time(0);
ERROR: date/time field value out of range: "24:00:00"which means that data originally accepted will fail to dump and reload.
I see this behavior in all versions back to 7.3. 7.2 was even more
broken:regression=# select '23:59:59.9'::time(0);
time
----------
00:00:00
(1 row)I think the correct behavior has to be to check for overflow again
after rounding off. Alternatively: why are we forbidding the value
24:00:00 anyway? Is there a reason not to allow the hours field
to exceed 23?regards, tom lane
---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073Bruce Momjian <pgman@candle.pha.pa.us> writes:
Where are we on this?
We haven't decided what to do.
I think my preference is to allow '24:00:00' (but not anything larger)
as a valid input value of the time datatypes. This for two reasons:
* existing dump files may contain such values
* it's consistent with allowing, eg, '12:13:60', which we
allow even though it's certainly not a valid leap second.
The alternative is to try to catch all places where 23:59:59.something
could get rounded up to 24:00:00, but that looks messy, and it would
introduce a gotcha into calculations on time values.
regards, tom lane
Tom Lane wrote:
Bruce Momjian <pgman@candle.pha.pa.us> writes:
Where are we on this?
We haven't decided what to do.
I think my preference is to allow '24:00:00' (but not anything larger)
as a valid input value of the time datatypes. This for two reasons:
* existing dump files may contain such values
* it's consistent with allowing, eg, '12:13:60', which we
allow even though it's certainly not a valid leap second.The alternative is to try to catch all places where 23:59:59.something
could get rounded up to 24:00:00, but that looks messy, and it would
introduce a gotcha into calculations on time values.
Is this a must-fix for 8.1?
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073Bruce Momjian <pgman@candle.pha.pa.us> writes:
Tom Lane wrote:
I think my preference is to allow '24:00:00' (but not anything larger)
as a valid input value of the time datatypes.
Is this a must-fix for 8.1?
No, since it's a pre-existing issue, but it's the kind of thing that
should be changed during a major release not a point-release. If we
don't change it then I think we'd have to wait till 8.2 before doing
anything about it.
regards, tom lane
Tom,
I think my preference is to allow '24:00:00' (but not anything larger)
as a valid input value of the time datatypes. This for two reasons:
* existing dump files may contain such values
* it's consistent with allowing, eg, '12:13:60', which we
allow even though it's certainly not a valid leap second.
It's also consistent with how several other RDBMSes do things (SQL Server,
MySQL), and several programming languages.
--
--Josh
Josh Berkus
Aglio Database Solutions
San Francisco
I have written the attached patch which I think does what you suggested.
I found all the places where we disallowed 24:00:00, and make it valid,
including nabstime.c.
test=> select '24:00:00'::time(0);
time
----------
24:00:00
(1 row)
test=> select '24:00:01'::time(0);
ERROR: date/time field value out of range: "24:00:01"
---------------------------------------------------------------------------
Tom Lane wrote:
Inserting into a time field with limited precision rounds off, which
is good except for this case:regression=# select '23:59:59.9'::time(0);
time
----------
24:00:00
(1 row)This is bad because:
regression=# select '24:00:00'::time(0);
ERROR: date/time field value out of range: "24:00:00"which means that data originally accepted will fail to dump and reload.
I see this behavior in all versions back to 7.3. 7.2 was even more
broken:regression=# select '23:59:59.9'::time(0);
time
----------
00:00:00
(1 row)I think the correct behavior has to be to check for overflow again
after rounding off. Alternatively: why are we forbidding the value
24:00:00 anyway? Is there a reason not to allow the hours field
to exceed 23?regards, tom lane
---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073Attachments:
/pgpatches/timetext/plainDownload
Index: src/backend/utils/adt/datetime.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/adt/datetime.c,v
retrieving revision 1.158
diff -c -c -r1.158 datetime.c
*** src/backend/utils/adt/datetime.c 9 Oct 2005 17:21:46 -0000 1.158
--- src/backend/utils/adt/datetime.c 14 Oct 2005 02:52:39 -0000
***************
*** 1114,1120 ****
* Check upper limit on hours; other limits checked in
* DecodeTime()
*/
! if (tm->tm_hour > 23)
return DTERR_FIELD_OVERFLOW;
break;
--- 1114,1122 ----
* Check upper limit on hours; other limits checked in
* DecodeTime()
*/
! /* test for > 24:00:00 */
! if (tm->tm_hour > 24 ||
! (tm->tm_hour == 24 && (tm->tm_min > 0 || tm->tm_sec > 0)))
return DTERR_FIELD_OVERFLOW;
break;
***************
*** 2243,2256 ****
else if (mer == PM && tm->tm_hour != 12)
tm->tm_hour += 12;
#ifdef HAVE_INT64_TIMESTAMP
! if (tm->tm_hour < 0 || tm->tm_hour > 23 || tm->tm_min < 0 ||
! tm->tm_min > 59 || tm->tm_sec < 0 || tm->tm_sec > 60 ||
*fsec < INT64CONST(0) || *fsec >= USECS_PER_SEC)
return DTERR_FIELD_OVERFLOW;
#else
! if (tm->tm_hour < 0 || tm->tm_hour > 23 || tm->tm_min < 0 ||
! tm->tm_min > 59 || tm->tm_sec < 0 || tm->tm_sec > 60 ||
*fsec < 0 || *fsec >= 1)
return DTERR_FIELD_OVERFLOW;
#endif
--- 2245,2260 ----
else if (mer == PM && tm->tm_hour != 12)
tm->tm_hour += 12;
+ 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 ||
+ /* Allow 24:00:00 */
+ (tm->tm_hour == 24 && (tm->tm_min > 0 || tm->tm_sec > 0 ||
#ifdef HAVE_INT64_TIMESTAMP
! *fsec > INT64CONST(0))) ||
*fsec < INT64CONST(0) || *fsec >= USECS_PER_SEC)
return DTERR_FIELD_OVERFLOW;
#else
! *fsec > 0)) ||
*fsec < 0 || *fsec >= 1)
return DTERR_FIELD_OVERFLOW;
#endif
Index: src/backend/utils/adt/nabstime.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/adt/nabstime.c,v
retrieving revision 1.143
diff -c -c -r1.143 nabstime.c
*** src/backend/utils/adt/nabstime.c 24 Sep 2005 22:54:38 -0000 1.143
--- src/backend/utils/adt/nabstime.c 14 Oct 2005 02:52:40 -0000
***************
*** 187,193 ****
if (tm->tm_year < 1901 || tm->tm_year > 2038
|| tm->tm_mon < 1 || tm->tm_mon > 12
|| tm->tm_mday < 1 || tm->tm_mday > 31
! || tm->tm_hour < 0 || tm->tm_hour > 23
|| tm->tm_min < 0 || tm->tm_min > 59
|| tm->tm_sec < 0 || tm->tm_sec > 60)
return INVALID_ABSTIME;
--- 187,195 ----
if (tm->tm_year < 1901 || tm->tm_year > 2038
|| tm->tm_mon < 1 || tm->tm_mon > 12
|| tm->tm_mday < 1 || tm->tm_mday > 31
! || tm->tm_hour < 0
! || tm->tm_hour > 24 /* Allow 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)
return INVALID_ABSTIME;
Index: src/interfaces/ecpg/pgtypeslib/dt_common.c
===================================================================
RCS file: /cvsroot/pgsql/src/interfaces/ecpg/pgtypeslib/dt_common.c,v
retrieving revision 1.30
diff -c -c -r1.30 dt_common.c
*** src/interfaces/ecpg/pgtypeslib/dt_common.c 9 Oct 2005 17:21:45 -0000 1.30
--- src/interfaces/ecpg/pgtypeslib/dt_common.c 14 Oct 2005 02:52:41 -0000
***************
*** 2095,2101 ****
* Check upper limit on hours; other limits checked in
* DecodeTime()
*/
! if (tm->tm_hour > 23)
return -1;
break;
--- 2095,2103 ----
* Check upper limit on hours; other limits checked in
* DecodeTime()
*/
! /* test for > 24:00:00 */
! if (tm->tm_hour > 24 ||
! (tm->tm_hour == 24 && (tm->tm_min > 0 || tm->tm_sec > 0)))
return -1;
break;
***************
*** 3161,3167 ****
err = 1;
*minute = 0;
}
! if (*hour > 23)
{
err = 1;
*hour = 0;
--- 3163,3170 ----
err = 1;
*minute = 0;
}
! if (*hour > 24 ||
! (*hour == 24 && (*minute > 0 || *second > 0)))
{
err = 1;
*hour = 0;
Bruce Momjian <pgman@candle.pha.pa.us> writes:
I have written the attached patch which I think does what you suggested.
I found all the places where we disallowed 24:00:00, and make it valid,
including nabstime.c.
Looks reasonable right offhand ... don't forget to update the docs too.
regards, tom lane
Patch applied, with documentation updates.
---------------------------------------------------------------------------
Tom Lane wrote:
Inserting into a time field with limited precision rounds off, which
is good except for this case:regression=# select '23:59:59.9'::time(0);
time
----------
24:00:00
(1 row)This is bad because:
regression=# select '24:00:00'::time(0);
ERROR: date/time field value out of range: "24:00:00"which means that data originally accepted will fail to dump and reload.
I see this behavior in all versions back to 7.3. 7.2 was even more
broken:regression=# select '23:59:59.9'::time(0);
time
----------
00:00:00
(1 row)I think the correct behavior has to be to check for overflow again
after rounding off. Alternatively: why are we forbidding the value
24:00:00 anyway? Is there a reason not to allow the hours field
to exceed 23?regards, tom lane
---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073Attachments:
/pgpatches/timetext/plainDownload
Index: doc/src/sgml/datatype.sgml
===================================================================
RCS file: /cvsroot/pgsql/doc/src/sgml/datatype.sgml,v
retrieving revision 1.160
diff -c -c -r1.160 datatype.sgml
*** doc/src/sgml/datatype.sgml 2 Oct 2005 23:50:06 -0000 1.160
--- doc/src/sgml/datatype.sgml 14 Oct 2005 03:09:07 -0000
***************
*** 1368,1374 ****
<entry>8 bytes</entry>
<entry>times of day only</entry>
<entry>00:00:00.00</entry>
! <entry>23:59:59.99</entry>
<entry>1 microsecond / 14 digits</entry>
</row>
<row>
--- 1368,1374 ----
<entry>8 bytes</entry>
<entry>times of day only</entry>
<entry>00:00:00.00</entry>
! <entry>24:00:00</entry>
<entry>1 microsecond / 14 digits</entry>
</row>
<row>
***************
*** 1376,1382 ****
<entry>12 bytes</entry>
<entry>times of day only, with time zone</entry>
<entry>00:00:00.00+12</entry>
! <entry>23:59:59.99-12</entry>
<entry>1 microsecond / 14 digits</entry>
</row>
</tbody>
--- 1376,1382 ----
<entry>12 bytes</entry>
<entry>times of day only, with time zone</entry>
<entry>00:00:00.00+12</entry>
! <entry>24:00:00-12</entry>
<entry>1 microsecond / 14 digits</entry>
</row>
</tbody>
Index: src/backend/utils/adt/datetime.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/adt/datetime.c,v
retrieving revision 1.158
diff -c -c -r1.158 datetime.c
*** src/backend/utils/adt/datetime.c 9 Oct 2005 17:21:46 -0000 1.158
--- src/backend/utils/adt/datetime.c 14 Oct 2005 03:09:09 -0000
***************
*** 1114,1120 ****
* Check upper limit on hours; other limits checked in
* DecodeTime()
*/
! if (tm->tm_hour > 23)
return DTERR_FIELD_OVERFLOW;
break;
--- 1114,1122 ----
* Check upper limit on hours; other limits checked in
* DecodeTime()
*/
! /* test for > 24:00:00 */
! if (tm->tm_hour > 24 ||
! (tm->tm_hour == 24 && (tm->tm_min > 0 || tm->tm_sec > 0)))
return DTERR_FIELD_OVERFLOW;
break;
***************
*** 2243,2256 ****
else if (mer == PM && tm->tm_hour != 12)
tm->tm_hour += 12;
#ifdef HAVE_INT64_TIMESTAMP
! if (tm->tm_hour < 0 || tm->tm_hour > 23 || tm->tm_min < 0 ||
! tm->tm_min > 59 || tm->tm_sec < 0 || tm->tm_sec > 60 ||
*fsec < INT64CONST(0) || *fsec >= USECS_PER_SEC)
return DTERR_FIELD_OVERFLOW;
#else
! if (tm->tm_hour < 0 || tm->tm_hour > 23 || tm->tm_min < 0 ||
! tm->tm_min > 59 || tm->tm_sec < 0 || tm->tm_sec > 60 ||
*fsec < 0 || *fsec >= 1)
return DTERR_FIELD_OVERFLOW;
#endif
--- 2245,2260 ----
else if (mer == PM && tm->tm_hour != 12)
tm->tm_hour += 12;
+ 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 ||
+ /* test for > 24:00:00 */
+ (tm->tm_hour == 24 && (tm->tm_min > 0 || tm->tm_sec > 0 ||
#ifdef HAVE_INT64_TIMESTAMP
! *fsec > INT64CONST(0))) ||
*fsec < INT64CONST(0) || *fsec >= USECS_PER_SEC)
return DTERR_FIELD_OVERFLOW;
#else
! *fsec > 0)) ||
*fsec < 0 || *fsec >= 1)
return DTERR_FIELD_OVERFLOW;
#endif
Index: src/backend/utils/adt/nabstime.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/adt/nabstime.c,v
retrieving revision 1.143
diff -c -c -r1.143 nabstime.c
*** src/backend/utils/adt/nabstime.c 24 Sep 2005 22:54:38 -0000 1.143
--- src/backend/utils/adt/nabstime.c 14 Oct 2005 03:09:09 -0000
***************
*** 184,195 ****
AbsoluteTime sec;
/* 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_mday < 1 || tm->tm_mday > 31
! || tm->tm_hour < 0 || tm->tm_hour > 23
! || tm->tm_min < 0 || tm->tm_min > 59
! || tm->tm_sec < 0 || tm->tm_sec > 60)
return INVALID_ABSTIME;
day = date2j(tm->tm_year, tm->tm_mon, tm->tm_mday) - UNIX_EPOCH_JDATE;
--- 184,197 ----
AbsoluteTime sec;
/* 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_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)
return INVALID_ABSTIME;
day = date2j(tm->tm_year, tm->tm_mon, tm->tm_mday) - UNIX_EPOCH_JDATE;
Index: src/interfaces/ecpg/pgtypeslib/dt_common.c
===================================================================
RCS file: /cvsroot/pgsql/src/interfaces/ecpg/pgtypeslib/dt_common.c,v
retrieving revision 1.30
diff -c -c -r1.30 dt_common.c
*** src/interfaces/ecpg/pgtypeslib/dt_common.c 9 Oct 2005 17:21:45 -0000 1.30
--- src/interfaces/ecpg/pgtypeslib/dt_common.c 14 Oct 2005 03:09:11 -0000
***************
*** 2095,2101 ****
* Check upper limit on hours; other limits checked in
* DecodeTime()
*/
! if (tm->tm_hour > 23)
return -1;
break;
--- 2095,2103 ----
* Check upper limit on hours; other limits checked in
* DecodeTime()
*/
! /* test for > 24:00:00 */
! if (tm->tm_hour > 24 ||
! (tm->tm_hour == 24 && (tm->tm_min > 0 || tm->tm_sec > 0)))
return -1;
break;
***************
*** 3161,3167 ****
err = 1;
*minute = 0;
}
! if (*hour > 23)
{
err = 1;
*hour = 0;
--- 3163,3170 ----
err = 1;
*minute = 0;
}
! if (*hour > 24 || /* test for > 24:00:00 */
! (*hour == 24 && (*minute > 0 || *second > 0)))
{
err = 1;
*hour = 0;
On Thu, 2005-13-10 at 22:54 -0400, Bruce Momjian wrote:
I have written the attached patch which I think does what you suggested.
I found all the places where we disallowed 24:00:00, and make it valid,
including nabstime.c.
Should this be added to the regression tests?
-Neil
Neil Conway wrote:
On Thu, 2005-13-10 at 22:54 -0400, Bruce Momjian wrote:
I have written the attached patch which I think does what you suggested.
I found all the places where we disallowed 24:00:00, and make it valid,
including nabstime.c.Should this be added to the regression tests?
Yes.
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
On 10/13/05, Josh Berkus <josh@agliodbs.com> wrote:
Tom,
I think my preference is to allow '24:00:00' (but not anything larger)
as a valid input value of the time datatypes. This for two reasons:
* existing dump files may contain such values
* it's consistent with allowing, eg, '12:13:60', which we
allow even though it's certainly not a valid leap second.
we shouldn't be allowing such timestamps! We should enforce only the
canonical formats of any datatype. Imagine what chaos would have been
caused if we didn't have IEEE specifications for the floating point
numbers!!!
It's also consistent with how several other RDBMSes do things (SQL Server,
MySQL), and several programming languages.
Just wanted to note that this is not really consistent with other
databases. For eg. SQL Server's o/p is shown below.
select convert( datetime, '23:59:59.998' )
1900-01-01 23:59:59.997
select convert( datetime, '23:59:59.999' )
1900-01-02 00:00:00.000 /* the date changes but the time remains
under 24:00:00 */
select convert( datetime, '24:00:00' )
Server: Msg 242, Level 16, State 3, Line 1
The conversion of a char data type to a datetime data type resulted in
an out-of-range datetime value.
Moreover, 24:00:00 not in canonical format so it should not be encoraged at all.
Gujreet.