BUG #1609: Bug in interval datatype for 64 Bit timestamps

"Oliver Siegmar" <oliver@siegmar.net> writes:

select '10.10 secs ago'::interval;

interval
-------------------
@ 10.-10 secs ago
(1 row)

What datestyle are you using?

regards, tom lane

Hi Tom,

On Wednesday 20 April 2005 17:57, Tom Lane wrote:

"Oliver Siegmar" <oliver@siegmar.net> writes:

select '10.10 secs ago'::interval;

interval
-------------------
@ 10.-10 secs ago
(1 row)

What datestyle are you using?

Non-ISO (Postgres in that case), but the handling for non-ISO is all the same
in interval.c ...

ISO works fine -

interval
--------------
-00:00:10.10
(1 row)

Oliver

Oliver Siegmar <oliver@siegmar.net> writes:

What datestyle are you using?

Non-ISO (Postgres in that case), but the handling for non-ISO is all the same
in interval.c ...

Yeah, I just confirmed here that it's broken the same way in all three
non-ISO datestyles. Will look into a fix later today.

regards, tom lane

I've applied this patch.

regards, tom lane

Index: datetime.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/adt/datetime.c,v
retrieving revision 1.137
diff -c -r1.137 datetime.c
*** datetime.c	11 Jan 2005 18:33:45 -0000	1.137
--- datetime.c	20 Apr 2005 17:09:57 -0000
***************
*** 3883,3899 ****
  			/* fractional seconds? */
  			if (fsec != 0)
  			{
  #ifdef HAVE_INT64_TIMESTAMP
  				if (is_before || ((!is_nonzero) && (tm->tm_sec < 0)))
  					tm->tm_sec = -tm->tm_sec;
  				sprintf(cp, "%s%d.%02d secs", (is_nonzero ? " " : ""),
! 						tm->tm_sec, (((int) fsec) / 10000));
  				cp += strlen(cp);
- 				if (!is_nonzero)
- 					is_before = (fsec < 0);
  #else
- 				fsec_t		sec;
- 
  				fsec += tm->tm_sec;
  				sec = fsec;
  				if (is_before || ((!is_nonzero) && (fsec < 0)))
--- 3883,3907 ----
  			/* fractional seconds? */
  			if (fsec != 0)
  			{
+ 				fsec_t		sec;
+ 
  #ifdef HAVE_INT64_TIMESTAMP
+ 				sec = fsec;
  				if (is_before || ((!is_nonzero) && (tm->tm_sec < 0)))
+ 				{
  					tm->tm_sec = -tm->tm_sec;
+ 					sec = -sec;
+ 					is_before = TRUE;
+ 				}
+ 				else if ((!is_nonzero) && (tm->tm_sec == 0) && (fsec < 0))
+ 				{
+ 					sec = -sec;
+ 					is_before = TRUE;
+ 				}
  				sprintf(cp, "%s%d.%02d secs", (is_nonzero ? " " : ""),
! 						tm->tm_sec, (((int) sec) / 10000));
  				cp += strlen(cp);
  #else
  				fsec += tm->tm_sec;
  				sec = fsec;
  				if (is_before || ((!is_nonzero) && (fsec < 0)))
***************
*** 3905,3913 ****
  					is_before = (fsec < 0);
  #endif
  				is_nonzero = TRUE;
- 
- 				/* otherwise, integer seconds only? */
  			}
  			else if (tm->tm_sec != 0)
  			{
  				int			sec = tm->tm_sec;
--- 3913,3920 ----
  					is_before = (fsec < 0);
  #endif
  				is_nonzero = TRUE;
  			}
+ 			/* otherwise, integer seconds only? */
  			else if (tm->tm_sec != 0)
  			{
  				int			sec = tm->tm_sec;

I've applied this patch.

It removed the bug, but also added a new one (hopefully only one ;-))

...now with ISO DateStyle -

select '2005 years 4 mons 20 days 15 hours 57 mins 12.1 secs ago'::interval;

Before your patch:

interval
-------------------------------------------
-2005 years -4 mons -20 days -15:57:12.10
(1 row)

after applying your patch:

interval
---------------------------------------------------
-2005 years -4 mons -20 days -15:57:12.1000000001
(1 row)

Oliver

Oliver Siegmar <oliver@siegmar.net> writes:

It removed the bug, but also added a new one (hopefully only one ;-))

I don't think it's a new bug, seeing that I didn't change the code
for the ISO case.

I see the imprecise result only in the non-integer-datetime case; is
it acting differently for you?

If it is only the float case, some imprecision is to be expected.

regards, tom lane

On Thursday 21 April 2005 15:57, Tom Lane wrote:

I don't think it's a new bug, seeing that I didn't change the code
for the ISO case.

I see the imprecise result only in the non-integer-datetime case; is
it acting differently for you?

You're right - it has nothing to do with your patch. I thought that because I
had the problem on one system (with your patch) but not on another (without
your patch). But these systems are having another difference - 64bit integer
datetimes.

I have this imprecise only on the non-integer-datetime server, too.

If it is only the float case, some imprecision is to be expected.

So everything is okay?

Oliver

Oliver Siegmar <oliver@siegmar.net> writes:

On Thursday 21 April 2005 15:57, Tom Lane wrote:

If it is only the float case, some imprecision is to be expected.

So everything is okay?

Well, it's not necessarily *wrong*, but maybe we could improve it.
The code currently assumes it can print 10 fractional digits in the
float case, which is overly optimistic once you get a large number
of days in the "days" component. Maybe we should add some code
to back off the precision depending on the number of days?

regards, tom lane

Tom Lane wrote:

Oliver Siegmar <oliver@siegmar.net> writes:

On Thursday 21 April 2005 15:57, Tom Lane wrote:

If it is only the float case, some imprecision is to be expected.

So everything is okay?

Well, it's not necessarily *wrong*, but maybe we could improve it.
The code currently assumes it can print 10 fractional digits in the
float case, which is overly optimistic once you get a large number
of days in the "days" component. Maybe we should add some code
to back off the precision depending on the number of days?

Well, it seems strange to change the display based on the number of
days, but on the other hand this is how exponential numbers are
displayed, with an X.YYYY EZZ so I suppose it does make sense to
suppress some of the fractional seconds for a large number of days.

I assume we would have to document this behavior. How do we determine
the range to adjust?

-- 
  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

Tom Lane wrote:

Oliver Siegmar <oliver@siegmar.net> writes:

On Thursday 21 April 2005 15:57, Tom Lane wrote:

If it is only the float case, some imprecision is to be expected.

So everything is okay?

Well, it's not necessarily *wrong*, but maybe we could improve it.
The code currently assumes it can print 10 fractional digits in the
float case, which is overly optimistic once you get a large number
of days in the "days" component. Maybe we should add some code
to back off the precision depending on the number of days?

I decided to trim off just the last digit to show only 9 digits instead
of 10. The logic is that the last digit is giving us problems, and 9
digits is nano-second resolution, while 10 is 100 picoseconds, which is
kind of a weird default.

It does fix the problem:

test=> select '2005 years 4 mons 20 days 15 hours 57 mins 12.1 secs
ago'::interval;
interval
-------------------------------------------
-2005 years -4 mons -20 days -15:57:12.10
(1 row)

Doing like 200 days still shows the failure:

test=> select '2005 years 4 mons 200 days 15 hours 57 mins 12.1 secs
ago'::interval;
interval
---------------------------------------------------
-2005 years -4 mons -200 days -15:57:12.100000001
(1 row)

but I figure 20 days is much more common than 200.

-- 
  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