Re: [BUGS] BUG #1927: incorrect timestamp returned - Mailing list pgsql-patches
From | Tom Lane |
---|---|
Subject | Re: [BUGS] BUG #1927: incorrect timestamp returned |
Date | |
Msg-id | 29352.1128794292@sss.pgh.pa.us Whole thread Raw |
In response to | Re: [BUGS] BUG #1927: incorrect timestamp returned (Bruce Momjian <pgman@candle.pha.pa.us>) |
List | pgsql-patches |
Bruce Momjian <pgman@candle.pha.pa.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))
pgsql-patches by date: