Re: [BUGS] BUG #1927: incorrect timestamp returned

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Tom Lane wrote:
>> I think the solution is that timestamp_out needs to decide how many
>> fractional digits it wants to display, and then round off the input
>> accordingly, *before* it breaks the input down into y/m/d/h/m/s fields.
>> This "60.00" business is happening because the rounding is done only on
>> the seconds-and-fractional-seconds field.

> Well, the testing showed that the one with the most 9's was actually
> rounded up to a whole number by timestamp_in, meaning we never have a
> chance to adjust it in timestamp_out. I am assuming you will be able to
> round the middle test value up to a whole number in timestamp_out so the
> 60 number will disappear.

After further thought I've realized that the current methodology for
applying JROUND (ie, to apply it to a whole timestamp or time value)
is fundamentally bogus. The macro is trying to round off to six fraction
digits, but if you've got many digits on the left side of the decimal point
then you cannot guarantee that six fraction digits of precision are
available to work with. It is *only* sensible to JROUND a value
representing a fractional-seconds field (or possibly seconds +
fractional seconds). Accordingly, the majority of the JROUND calls in
the existing code are indeed bogus and should go away. The only one
that's not bogus is the one in dt2time(), which is properly applied to a
fractional second ... but the problem with that one is that there's no
provision for coping with the possibility that the fractional second
rounds up to 1.0. In that situation, we need to be able to propagate
the roundoff to higher fields to avoid the "23:59:60" syndrome.

Since dt2time doesn't have a way to propagate the roundoff into the date
part, we can't fix this right there. It needs to be handled at the
caller level (timestamp2tm and related routines).

Attached is a proposed patch that cleans this up. It's not quite
complete because I haven't propagated the changes into ecpglib,
but I thought I could put it out for comment in this form.

The patch does the following:

1. Replace JROUND with TSROUND (to round to MAX_TIMESTAMP_PRECISION
places) and TIMEROUND (to round to MAX_TIME_PRECISION places).

2. Remove JROUND calls in calculational routines.

3. In timestamp2tm, time2tm, timetz2tm, and interval2tm, round off
the fractional-seconds field to the right number of places, check
for rounding up to 1, and cope as needed.

4. Make the sprintf field width in EncodeTimeOnly match
MAX_TIME_PRECISION. Because it was only printing 9 digits and not 10,
there were cases where a properly rounded fractional time would still
print as "60.0". (I think at some point we reduced it from 10 to 9
as a hacky way of dealing with a different bug report ... but now I
feel we've gotten the right solution at last.)

The patch passes the regression tests and appears to fix the problems.

Comments?

regards, tom lane

*** contrib/btree_gist/btree_ts.c.orig Fri Jul 1 09:44:55 2005
--- contrib/btree_gist/btree_ts.c Sat Oct 8 13:12:37 2005
***************
*** 122,130 ****
*gmt -= (tz * INT64CONST(1000000));
#else
*gmt -= tz;
- *gmt = JROUND(*gmt);
#endif
-
}
return gmt;
}
--- 122,128 ----
*** src/backend/utils/adt/date.c.orig Thu Sep 8 22:31:49 2005
--- src/backend/utils/adt/date.c Sat Oct 8 13:12:45 2005
***************
*** 944,953 ****
--- 944,961 ----
#else
double trem;

+ recalc:
trem = time;
TMODULO(trem, tm->tm_hour, (double)SECS_PER_HOUR);
TMODULO(trem, tm->tm_min, (double)SECS_PER_MINUTE);
TMODULO(trem, tm->tm_sec, 1.0);
+ trem = TIMEROUND(trem);
+ /* roundoff may need to propagate to higher-order fields */
+ if (trem >= 1.0)
+ {
+ time = ceil(time);
+ goto recalc;
+ }
*fsec = trem;
#endif

***************
*** 1837,1845 ****
--- 1845,1861 ----
#else
double trem = time->time;

+ recalc:
TMODULO(trem, tm->tm_hour, (double)SECS_PER_HOUR);
TMODULO(trem, tm->tm_min, (double)SECS_PER_MINUTE);
TMODULO(trem, tm->tm_sec, 1.0);
+ trem = TIMEROUND(trem);
+ /* roundoff may need to propagate to higher-order fields */
+ if (trem >= 1.0)
+ {
+ trem = ceil(time->time);
+ goto recalc;
+ }
*fsec = trem;
#endif

*** src/backend/utils/adt/datetime.c.orig Sat Jul 23 10:25:33 2005
--- src/backend/utils/adt/datetime.c Sat Oct 8 13:12:46 2005
***************
*** 3488,3495 ****
sprintf(str, "%02d:%02d", tm->tm_hour, tm->tm_min);

/*
! * Print fractional seconds if any. The field widths here should be
! * at least equal to the larger of MAX_TIME_PRECISION and
* MAX_TIMESTAMP_PRECISION.
*/
if (fsec != 0)
--- 3488,3495 ----
sprintf(str, "%02d:%02d", tm->tm_hour, tm->tm_min);

/*
! * Print fractional seconds if any. The fractional field widths
! * here should be equal to the larger of MAX_TIME_PRECISION and
* MAX_TIMESTAMP_PRECISION.
*/
if (fsec != 0)
***************
*** 3497,3503 ****
#ifdef HAVE_INT64_TIMESTAMP
sprintf(str + strlen(str), ":%02d.%06d", tm->tm_sec, fsec);
#else
! sprintf(str + strlen(str), ":%012.9f", tm->tm_sec + fsec);
#endif
TrimTrailingZeros(str);
}
--- 3497,3503 ----
#ifdef HAVE_INT64_TIMESTAMP
sprintf(str + strlen(str), ":%02d.%06d", tm->tm_sec, fsec);
#else
! sprintf(str + strlen(str), ":%013.10f", tm->tm_sec + fsec);
#endif
TrimTrailingZeros(str);
}
*** src/backend/utils/adt/timestamp.c.orig Fri Sep 9 02:46:14 2005
--- src/backend/utils/adt/timestamp.c Sat Oct 8 13:35:21 2005
***************
*** 998,1007 ****
*min = time / SECS_PER_MINUTE;
time -= (*min) * SECS_PER_MINUTE;
*sec = time;
! *fsec = JROUND(time - *sec);
#endif
-
- return;
} /* dt2time() */


--- 998,1005 ----
*min = time / SECS_PER_MINUTE;
time -= (*min) * SECS_PER_MINUTE;
*sec = time;
! *fsec = time - *sec;
#endif
} /* dt2time() */


***************
*** 1038,1045 ****
#endif
}

- time = dt;
#ifdef HAVE_INT64_TIMESTAMP
TMODULO(time, date, USECS_PER_DAY);

if (time < INT64CONST(0))
--- 1036,1043 ----
#endif
}

#ifdef HAVE_INT64_TIMESTAMP
+ time = dt;
TMODULO(time, date, USECS_PER_DAY);

if (time < INT64CONST(0))
***************
*** 1047,1072 ****
time += USECS_PER_DAY;
date -= 1;
}
#else
TMODULO(time, date, (double)SECS_PER_DAY);

if (time < 0)
{
time += SECS_PER_DAY;
! date -=1;
}
- #endif

/* add offset to go from J2000 back to standard Julian date */
date += POSTGRES_EPOCH_JDATE;

/* Julian day routine does not work for negative Julian days */
! if (date <0 || date >(Timestamp) INT_MAX)
return -1;

j2date((int) date, &tm->tm_year, &tm->tm_mon, &tm->tm_mday);
dt2time(time, &tm->tm_hour, &tm->tm_min, &tm->tm_sec, fsec);

/* Done if no TZ conversion wanted */
if (tzp == NULL)
{
--- 1045,1097 ----
time += USECS_PER_DAY;
date -= 1;
}
+
+ /* add offset to go from J2000 back to standard Julian date */
+ date += POSTGRES_EPOCH_JDATE;
+
+ /* Julian day routine does not work for negative Julian days */
+ if (date < 0 || date > (Timestamp) INT_MAX)
+ return -1;
+
+ j2date((int) date, &tm->tm_year, &tm->tm_mon, &tm->tm_mday);
+ dt2time(time, &tm->tm_hour, &tm->tm_min, &tm->tm_sec, fsec);
#else
+ time = dt;
TMODULO(time, date, (double)SECS_PER_DAY);

if (time < 0)
{
time += SECS_PER_DAY;
! date -= 1;
}

/* add offset to go from J2000 back to standard Julian date */
date += POSTGRES_EPOCH_JDATE;

+ recalc_d:
/* Julian day routine does not work for negative Julian days */
! if (date < 0 || date > (Timestamp) INT_MAX)
return -1;

j2date((int) date, &tm->tm_year, &tm->tm_mon, &tm->tm_mday);
+ recalc_t:
dt2time(time, &tm->tm_hour, &tm->tm_min, &tm->tm_sec, fsec);

+ *fsec = TSROUND(*fsec);
+ /* roundoff may need to propagate to higher-order fields */
+ if (*fsec >= 1.0)
+ {
+ time = ceil(time);
+ if (time >= (double)SECS_PER_DAY)
+ {
+ time = 0;
+ date += 1;
+ goto recalc_d;
+ }
+ goto recalc_t;
+ }
+ #endif
+
/* Done if no TZ conversion wanted */
if (tzp == NULL)
{
***************
*** 1216,1224 ****
--- 1241,1257 ----
tm->tm_sec = time / USECS_PER_SEC;
*fsec = time - (tm->tm_sec * USECS_PER_SEC);
#else
+ recalc:
TMODULO(time, tm->tm_hour, (double)SECS_PER_HOUR);
TMODULO(time, tm->tm_min, (double)SECS_PER_MINUTE);
TMODULO(time, tm->tm_sec, 1.0);
+ time = TSROUND(time);
+ /* roundoff may need to propagate to higher-order fields */
+ if (time >= 1.0)
+ {
+ time = ceil(span.time);
+ goto recalc;
+ }
*fsec = time;
#endif

***************
*** 1237,1244 ****
#else
span->time = (((tm->tm_hour * (double)MINS_PER_HOUR) +
tm->tm_min) * (double)SECS_PER_MINUTE) +
! tm->tm_sec;
! span->time = JROUND(span->time + fsec);
#endif

return 0;
--- 1270,1276 ----
#else
span->time = (((tm->tm_hour * (double)MINS_PER_HOUR) +
tm->tm_min) * (double)SECS_PER_MINUTE) +
! tm->tm_sec + fsec;
#endif

return 0;
***************
*** 1266,1272 ****
dt -= (tz * USECS_PER_SEC);
#else
dt -= tz;
- dt = JROUND(dt);
#endif
return dt;
} /* dt2local() */
--- 1298,1303 ----
***************
*** 1901,1911 ****
(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
errmsg("cannot subtract infinite timestamps")));

- #ifdef HAVE_INT64_TIMESTAMP
result->time = dt1 - dt2;
- #else
- result->time = JROUND(dt1 - dt2);
- #endif

result->month = 0;
result->day = 0;
--- 1932,1938 ----
***************
*** 2224,2234 ****

result->month = span1->month + span2->month;
result->day = span1->day + span2->day;
- #ifdef HAVE_INT64_TIMESTAMP
result->time = span1->time + span2->time;
- #else
- result->time = JROUND(span1->time + span2->time);
- #endif

PG_RETURN_INTERVAL_P(result);
}
--- 2251,2257 ----
***************
*** 2244,2254 ****

result->month = span1->month - span2->month;
result->day = span1->day - span2->day;
- #ifdef HAVE_INT64_TIMESTAMP
result->time = span1->time - span2->time;
- #else
- result->time = JROUND(span1->time - span2->time);
- #endif

PG_RETURN_INTERVAL_P(result);
}
--- 2267,2273 ----
***************
*** 2280,2286 ****
#ifdef HAVE_INT64_TIMESTAMP
result->time = rint(span->time * factor + day_remainder * USECS_PER_DAY);
#else
! result->time = JROUND(span->time * factor + day_remainder * SECS_PER_DAY);
#endif

result = DatumGetIntervalP(DirectFunctionCall1(interval_justify_hours,
--- 2299,2305 ----
#ifdef HAVE_INT64_TIMESTAMP
result->time = rint(span->time * factor + day_remainder * USECS_PER_DAY);
#else
! result->time = span->time * factor + day_remainder * SECS_PER_DAY;
#endif

result = DatumGetIntervalP(DirectFunctionCall1(interval_justify_hours,
***************
*** 2332,2338 ****
result->time += rint(day_remainder * USECS_PER_DAY);
#else
result->time += day_remainder * SECS_PER_DAY;
- result->time = JROUND(result->time);
#endif

result = DatumGetIntervalP(DirectFunctionCall1(interval_justify_hours,
--- 2351,2356 ----
*** src/include/utils/date.h.orig Fri Feb 25 11:13:29 2005
--- src/include/utils/date.h Sat Oct 8 13:12:28 2005
***************
*** 60,65 ****
--- 60,69 ----

#define MAX_TIME_PRECISION 10

+ /* round off to MAX_TIME_PRECISION decimal places */
+ #define TIME_PREC_INV 10000000000.0
+ #define TIMEROUND(j) (rint(((double) (j)) * TIME_PREC_INV) / TIME_PREC_INV)
+
#define DatumGetDateADT(X) ((DateADT) DatumGetInt32(X))
#define DatumGetTimeADT(X) ((TimeADT) DatumGetFloat8(X))
#define DatumGetTimeTzADTP(X) ((TimeTzADT *) DatumGetPointer(X))
*** src/include/utils/timestamp.h.orig Sat Oct 8 10:52:07 2005
--- src/include/utils/timestamp.h Sat Oct 8 13:35:06 2005
***************
*** 163,170 ****

typedef double fsec_t;

! #define TIME_PREC_INV 1000000.0
! #define JROUND(j) (rint(((double) (j)) * TIME_PREC_INV) / TIME_PREC_INV)
#endif

#define TIMESTAMP_MASK(b) (1 << (b))
--- 163,173 ----

typedef double fsec_t;

! /* round off to MAX_TIMESTAMP_PRECISION decimal places */
! /* note: this is also used for rounding off intervals */
! #define TS_PREC_INV 1000000.0
! #define TSROUND(j) (rint(((double) (j)) * TS_PREC_INV) / TS_PREC_INV)
!
#endif

#define TIMESTAMP_MASK(b) (1 << (b))