Thread: Re: [PATCHES] Interval aggregate regression failure
Bruce Momjian <bruce@momjian.us> writes: > I am unclear about this report. The patch was not meant to fix every > interval issue, but merely to improve multiplication and division > computations. Does it do that? According to Michael's last report, your patch fails under --enable-integer-datetimes. This is an issue where you have to be "as simple as possible, but no simpler". I think the approach you are proposing is too simple. Michael's last patch here: http://archives.postgresql.org/pgsql-patches/2006-08/msg00447.php looks considerably more likely to lead to a workable answer. regards, tom lane
Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > I am unclear about this report. The patch was not meant to fix every
> > interval issue, but merely to improve multiplication and division
> > computations. Does it do that?
>
> According to Michael's last report, your patch fails under
> --enable-integer-datetimes.
Where does it fail? Here?
select interval '41 mon 12 days 360:00' * 0.3 as product_a
, interval '-41 mon -12 days +360:00' * 0.3 as product_b
, interval '-41 mon 12 days 360:00' * 0.3 as product_c
, interval '-41 mon -12 days -360:00' * 0.3 as product_d;
product_a | product_b |
product_c | product_d
--------------------------+-----------------------------
+----------------------------+---------------------------------
1 year 11 days 146:24:00 | -1 years -11 days +69:36:00 | -1 years -5
days +98:24:00 | -1 years -11 days -146:23:60.00
-----
That is wrong, but I think we need another fix for that. Notice the
problem is in minutes/seconds, not hours.
> This is an issue where you have to be "as simple as possible, but no
> simpler". I think the approach you are proposing is too simple.
> Michael's last patch here:
> http://archives.postgresql.org/pgsql-patches/2006-08/msg00447.php
> looks considerably more likely to lead to a workable answer.
I see he is taking the fractional part of the result and finding if that
should round. I am confused why that would help the -146:23:60.00 value
above. Notice we only see it for negative values too.
I do like that he is rounding the computation spillover, and not the
total time value, which is what I started with.
I believe my provides a more accurate computation, and doesn't have the
problems of rounding. The only bug we can find is the powerpc one for
-146:23:60 minutes.
--
Bruce Momjian bruce@momjian.us
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
On Sep 1, 2006, at 11:31 , Bruce Momjian wrote: > Tom Lane wrote: >> Bruce Momjian <bruce@momjian.us> writes: >>> I am unclear about this report. The patch was not meant to fix >>> every >>> interval issue, but merely to improve multiplication and division >>> computations. Does it do that? >> >> According to Michael's last report, your patch fails under >> --enable-integer-datetimes. > > Where does it fail? Here? > > select interval '41 mon 12 days 360:00' * 0.3 as product_a > , interval '-41 mon -12 days +360:00' * 0.3 as product_b > , interval '-41 mon 12 days 360:00' * 0.3 as product_c > , interval '-41 mon -12 days -360:00' * 0.3 as product_d; > product_a | product_b | > product_c | product_d > --------------------------+----------------------------- > +----------------------------+--------------------------------- > 1 year 11 days 146:24:00 | -1 years -11 days +69:36:00 | -1 years -5 > days +98:24:00 | -1 years -11 days -146:23:60.00 > ----- > > That is wrong, but I think we need another fix for that. Notice the > problem is in minutes/seconds, not hours. I was sure it was more wrong than that the first time I saw it, but looks like I can't be sure of anything today :(. I need more sleep. Sorry for the noise on this one. Off work now, so I'm back at it. Michael Glaesemann grzm seespotcode net
Here's a patch that appears to work. Gives the same output with and
without --enable-integer-datetimes. Answers look like they're correct.
I'm basically treating the components as three different intervals
(with the other two components zero), rounding them each to usecs,
and adding them together.
While it might be nice to carry a little extra precision around, it
doesn't seem to be needed in these cases. If errors do arise, they
should be at most 3 usec, which is pretty much noise for the floating
point case, I suspect.
Bruce, how's it look on your machine? If it looks good, I'll add the
examples we've been using to the regression tests.
Michael Glaesemann
grzm seespotcode net
Index: src/backend/utils/adt/timestamp.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/utils/adt/timestamp.c,v
retrieving revision 1.165
diff -c -r1.165 timestamp.c
*** src/backend/utils/adt/timestamp.c 13 Jul 2006 16:49:16 -0000 1.165
--- src/backend/utils/adt/timestamp.c 1 Sep 2006 11:26:12 -0000
***************
*** 2494,2511 ****
float8 factor = PG_GETARG_FLOAT8(1);
double month_remainder,
day_remainder,
! month_remainder_days;
Interval *result;
result = (Interval *) palloc(sizeof(Interval));
! month_remainder = span->month * factor;
! day_remainder = span->day * factor;
result->month = (int32) month_remainder;
result->day = (int32) day_remainder;
month_remainder -= result->month;
day_remainder -= result->day;
/*
* The above correctly handles the whole-number part of the month
and day
* products, but we have to do something with any fractional part
--- 2494,2553 ----
float8 factor = PG_GETARG_FLOAT8(1);
double month_remainder,
day_remainder,
! month_remainder_days,
! month_remainder_time,
! day_remainder_time;
Interval *result;
result = (Interval *) palloc(sizeof(Interval));
!
! month_remainder = span->month / factor;
! day_remainder = span->day / factor;
result->month = (int32) month_remainder;
result->day = (int32) day_remainder;
month_remainder -= result->month;
day_remainder -= result->day;
+ month_remainder_days = month_remainder * DAYS_PER_MONTH;
+
+ /*
+ if month_remainder_days is not an integer, check to see if it's an
+ integer when converted to SECS or USECS.
+ If it is, round month_remainder_days to the nearest integer
+
+ */
+
+ if (month_remainder_days != (int32)month_remainder_days &&
+ TSROUND(month_remainder_days * SECS_PER_DAY) ==
+ rint(month_remainder_days * SECS_PER_DAY))
+ month_remainder_days = rint(month_remainder_days);
+
+ result->day += (int32)month_remainder_days;
+
+ #ifdef HAVE_INT64_TIMESTAMP
+ month_remainder_time = rint((month_remainder_days -
+ (int32)month_remainder_days) * USECS_PER_DAY);
+
+ day_remainder_time = rint(day_remainder * USECS_PER_DAY);
+
+
+ result->time = rint(rint(span->time * factor) + day_remainder_time +
+ month_remainder_time);
+ #else
+ month_remainder_time = rint((month_remainder_days -
+ (int32)month_remainder_days) * SECS_PER_DAY);
+ day_remainder_time = rint(day_remainder * SECS_PER_DAY);
+
+ result->time = span->time * factor + day_remainder_time +
+ month_remainder_time;
+ #endif
+
+
+ day_remainder = span->day * factor;
+ result->day = (int32) day_remainder;
+ day_remainder -= result->day;
+
/*
* The above correctly handles the whole-number part of the month
and day
* products, but we have to do something with any fractional part
***************
*** 2518,2531 ****
/* fractional months full days into days */
month_remainder_days = month_remainder * DAYS_PER_MONTH;
! result->day += (int32) month_remainder_days;
! /* fractional months partial days into time */
! day_remainder += month_remainder_days - (int32) month_remainder_days;
#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
PG_RETURN_INTERVAL_P(result);
--- 2560,2599 ----
/* fractional months full days into days */
month_remainder_days = month_remainder * DAYS_PER_MONTH;
! /*
! * The remainders suffer from float rounding, so if they are
! * within 1us of an integer, we round them to integers.
! * It seems doing two multiplies is causing the imprecision.
! */
#ifdef HAVE_INT64_TIMESTAMP
! if (month_remainder_days != (int32)month_remainder_days &&
! rint(month_remainder_days * USECS_PER_DAY) ==
! (int64)(month_remainder_days * USECS_PER_DAY))
! month_remainder_days = rint(month_remainder_days);
! result->day += (int32) month_remainder_days;
! month_remainder_time = rint((month_remainder_days -
! (int32)month_remainder_days) * USECS_PER_DAY);
!
! day_remainder_time = rint(day_remainder * USECS_PER_DAY);
!
! result->time = rint(span->time * factor + day_remainder_time +
! month_remainder_time);
#else
! if (month_remainder_days != (int32)month_remainder_days &&
! TSROUND(month_remainder_days * SECS_PER_DAY) ==
! rint(month_remainder_days * SECS_PER_DAY))
! month_remainder_days = rint(month_remainder_days);
! result->day += (int32) month_remainder_days;
!
! month_remainder_time = rint((month_remainder_days -
! (int32)month_remainder_days) * SECS_PER_DAY);
!
! day_remainder_time = rint(day_remainder * SECS_PER_DAY);
!
! result->time = span->time * factor + day_remainder_time +
! month_remainder_time;
!
#endif
PG_RETURN_INTERVAL_P(result);
***************
*** 2548,2554 ****
float8 factor = PG_GETARG_FLOAT8(1);
double month_remainder,
day_remainder,
! month_remainder_days;
Interval *result;
result = (Interval *) palloc(sizeof(Interval));
--- 2616,2624 ----
float8 factor = PG_GETARG_FLOAT8(1);
double month_remainder,
day_remainder,
! month_remainder_days,
! month_remainder_time,
! day_remainder_time;
Interval *result;
result = (Interval *) palloc(sizeof(Interval));
***************
*** 2571,2584 ****
/* fractional months full days into days */
month_remainder_days = month_remainder * DAYS_PER_MONTH;
result->day += (int32) month_remainder_days;
- /* fractional months partial days into time */
- day_remainder += month_remainder_days - (int32) month_remainder_days;
! #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
PG_RETURN_INTERVAL_P(result);
--- 2641,2680 ----
/* fractional months full days into days */
month_remainder_days = month_remainder * DAYS_PER_MONTH;
+ /*
+ * The remainders suffer from float rounding, so if they are
+ * within 1us of an integer, we round them to integers.
+ * It seems doing a division and multiplies is causing the
+ * imprecision.
+ */
+ if (month_remainder_days != (int32)month_remainder_days &&
+ TSROUND(month_remainder_days * SECS_PER_DAY) ==
+ rint(month_remainder_days * SECS_PER_DAY))
+ month_remainder_days = rint(month_remainder_days);
result->day += (int32) month_remainder_days;
! day_remainder_time = day_remainder * SECS_PER_DAY;
! if (day_remainder_time != (int32)day_remainder_time &&
! TSROUND(day_remainder_time) ==
! rint(day_remainder_time))
! day_remainder_time = rint(day_remainder_time);
!
! #ifdef HAVE_INT64_TIMESTAMP
! day_remainder_time = day_remainder * USECS_PER_DAY;
! if (day_remainder_time != (int32)day_remainder_time &&
! TSROUND(day_remainder_time) == rint(day_remainder_time))
! day_remainder_time = rint(day_remainder_time);
!
! result->time = rint(span->time / factor + day_remainder_time +
! (month_remainder_days - (int32)month_remainder_days) *
USECS_PER_DAY;
! #else
! day_remainder_time = day_remainder * SECS_PER_DAY;
! if (day_remainder_time != (int32)day_remainder_time &&
! TSROUND(day_remainder_time) == rint(day_remainder_time))
! day_remainder_time = rint(day_remainder_time);
!
! result->time = span->time / factor + day_remainder_time +
! (month_remainder_days - (int32)month_remainder_days) *
SECS_PER_DAY;
#endif
PG_RETURN_INTERVAL_P(result);
Index: src/include/utils/timestamp.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/utils/timestamp.h,v
retrieving revision 1.62
diff -c -r1.62 timestamp.h
*** src/include/utils/timestamp.h 13 Jul 2006 16:49:20 -0000 1.62
--- src/include/utils/timestamp.h 1 Sep 2006 11:26:15 -0000
***************
*** 161,171 ****
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))
#define INTERVAL_MASK(b) (1 << (b))
--- 161,175 ----
typedef double fsec_t;
! #endif
!
! /*
! * Round off to MAX_TIMESTAMP_PRECISION decimal places.
! * This is also used for rounding off intervals, and
! * for rounding interval multiplication/division calculations.
! */
#define TS_PREC_INV 1000000.0
#define TSROUND(j) (rint(((double) (j)) * TS_PREC_INV) / TS_PREC_INV)
#define TIMESTAMP_MASK(b) (1 << (b))
#define INTERVAL_MASK(b) (1 << (b))
On Sep 1, 2006, at 20:39 , Michael Glaesemann wrote:
> Here's a patch that appears to work. Gives the same output with and
> without --enable-integer-datetimes. Answers look like they're correct.
>
> I'm basically treating the components as three different intervals
> (with the other two components zero), rounding them each to usecs,
> and adding them together.
>
> While it might be nice to carry a little extra precision around, it
> doesn't seem to be needed in these cases. If errors do arise, they
> should be at most 3 usec, which is pretty much noise for the
> floating point case, I suspect.
Okay. Here's the patch. Bruce, does it work for you?
Michael Glaesemann
grzm seespotcode net
Index: src/backend/utils/adt/timestamp.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/utils/adt/timestamp.c,v
retrieving revision 1.165
diff -c -r1.165 timestamp.c
*** src/backend/utils/adt/timestamp.c 13 Jul 2006 16:49:16 -0000 1.165
--- src/backend/utils/adt/timestamp.c 1 Sep 2006 12:28:23 -0000
***************
*** 2494,2504 ****
float8 factor = PG_GETARG_FLOAT8(1);
double month_remainder,
day_remainder,
! month_remainder_days;
Interval *result;
result = (Interval *) palloc(sizeof(Interval));
month_remainder = span->month * factor;
day_remainder = span->day * factor;
result->month = (int32) month_remainder;
--- 2494,2507 ----
float8 factor = PG_GETARG_FLOAT8(1);
double month_remainder,
day_remainder,
! month_remainder_days,
! month_remainder_time,
! day_remainder_time;
Interval *result;
result = (Interval *) palloc(sizeof(Interval));
+
month_remainder = span->month * factor;
day_remainder = span->day * factor;
result->month = (int32) month_remainder;
***************
*** 2506,2511 ****
--- 2509,2553 ----
month_remainder -= result->month;
day_remainder -= result->day;
+ month_remainder_days = month_remainder * DAYS_PER_MONTH;
+
+ /*
+ if month_remainder_days is not an integer, check to see if it's an
+ integer when converted to SECS or USECS.
+ If it is, round month_remainder_days to the nearest integer
+
+ */
+
+ if (month_remainder_days != (int32)month_remainder_days &&
+ TSROUND(month_remainder_days * SECS_PER_DAY) ==
+ rint(month_remainder_days * SECS_PER_DAY))
+ month_remainder_days = rint(month_remainder_days);
+
+ result->day += (int32)month_remainder_days;
+
+ #ifdef HAVE_INT64_TIMESTAMP
+ month_remainder_time = rint((month_remainder_days -
+ (int32)month_remainder_days) * USECS_PER_DAY);
+
+ day_remainder_time = rint(day_remainder * USECS_PER_DAY);
+
+
+ result->time = rint(rint(span->time * factor) + day_remainder_time +
+ month_remainder_time);
+ #else
+ month_remainder_time = rint((month_remainder_days -
+ (int32)month_remainder_days) * SECS_PER_DAY);
+ day_remainder_time = rint(day_remainder * SECS_PER_DAY);
+
+ result->time = span->time * factor + day_remainder_time +
+ month_remainder_time;
+ #endif
+
+
+ day_remainder = span->day * factor;
+ result->day = (int32) day_remainder;
+ day_remainder -= result->day;
+
/*
* The above correctly handles the whole-number part of the month
and day
* products, but we have to do something with any fractional part
***************
*** 2518,2531 ****
/* fractional months full days into days */
month_remainder_days = month_remainder * DAYS_PER_MONTH;
! result->day += (int32) month_remainder_days;
! /* fractional months partial days into time */
! day_remainder += month_remainder_days - (int32) month_remainder_days;
#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
PG_RETURN_INTERVAL_P(result);
--- 2560,2599 ----
/* fractional months full days into days */
month_remainder_days = month_remainder * DAYS_PER_MONTH;
! /*
! * The remainders suffer from float rounding, so if they are
! * within 1us of an integer, we round them to integers.
! * It seems doing two multiplies is causing the imprecision.
! */
#ifdef HAVE_INT64_TIMESTAMP
! if (month_remainder_days != (int32)month_remainder_days &&
! rint(month_remainder_days * USECS_PER_DAY) ==
! (int64)(month_remainder_days * USECS_PER_DAY))
! month_remainder_days = rint(month_remainder_days);
! result->day += (int32) month_remainder_days;
! month_remainder_time = rint((month_remainder_days -
! (int32)month_remainder_days) * USECS_PER_DAY);
!
! day_remainder_time = rint(day_remainder * USECS_PER_DAY);
!
! result->time = rint(span->time * factor + day_remainder_time +
! month_remainder_time);
#else
! if (month_remainder_days != (int32)month_remainder_days &&
! TSROUND(month_remainder_days * SECS_PER_DAY) ==
! rint(month_remainder_days * SECS_PER_DAY))
! month_remainder_days = rint(month_remainder_days);
! result->day += (int32) month_remainder_days;
!
! month_remainder_time = rint((month_remainder_days -
! (int32)month_remainder_days) * SECS_PER_DAY);
!
! day_remainder_time = rint(day_remainder * SECS_PER_DAY);
!
! result->time = span->time * factor + day_remainder_time +
! month_remainder_time;
!
#endif
PG_RETURN_INTERVAL_P(result);
***************
*** 2548,2554 ****
float8 factor = PG_GETARG_FLOAT8(1);
double month_remainder,
day_remainder,
! month_remainder_days;
Interval *result;
result = (Interval *) palloc(sizeof(Interval));
--- 2616,2624 ----
float8 factor = PG_GETARG_FLOAT8(1);
double month_remainder,
day_remainder,
! month_remainder_days,
! month_remainder_time,
! day_remainder_time;
Interval *result;
result = (Interval *) palloc(sizeof(Interval));
***************
*** 2571,2584 ****
/* fractional months full days into days */
month_remainder_days = month_remainder * DAYS_PER_MONTH;
result->day += (int32) month_remainder_days;
- /* fractional months partial days into time */
- day_remainder += month_remainder_days - (int32) month_remainder_days;
! #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
PG_RETURN_INTERVAL_P(result);
--- 2641,2680 ----
/* fractional months full days into days */
month_remainder_days = month_remainder * DAYS_PER_MONTH;
+ /*
+ * The remainders suffer from float rounding, so if they are
+ * within 1us of an integer, we round them to integers.
+ * It seems doing a division and multiplies is causing the
+ * imprecision.
+ */
+ if (month_remainder_days != (int32)month_remainder_days &&
+ TSROUND(month_remainder_days * SECS_PER_DAY) ==
+ rint(month_remainder_days * SECS_PER_DAY))
+ month_remainder_days = rint(month_remainder_days);
result->day += (int32) month_remainder_days;
! day_remainder_time = day_remainder * SECS_PER_DAY;
! if (day_remainder_time != (int32)day_remainder_time &&
! TSROUND(day_remainder_time) ==
! rint(day_remainder_time))
! day_remainder_time = rint(day_remainder_time);
!
! #ifdef HAVE_INT64_TIMESTAMP
! day_remainder_time = day_remainder * USECS_PER_DAY;
! if (day_remainder_time != (int32)day_remainder_time &&
! TSROUND(day_remainder_time) == rint(day_remainder_time))
! day_remainder_time = rint(day_remainder_time);
!
! result->time = rint(span->time / factor + day_remainder_time +
! (month_remainder_days - (int32)month_remainder_days) *
USECS_PER_DAY;
! #else
! day_remainder_time = day_remainder * SECS_PER_DAY;
! if (day_remainder_time != (int32)day_remainder_time &&
! TSROUND(day_remainder_time) == rint(day_remainder_time))
! day_remainder_time = rint(day_remainder_time);
!
! result->time = span->time / factor + day_remainder_time +
! (month_remainder_days - (int32)month_remainder_days) *
SECS_PER_DAY;
#endif
PG_RETURN_INTERVAL_P(result);
Index: src/include/utils/timestamp.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/utils/timestamp.h,v
retrieving revision 1.62
diff -c -r1.62 timestamp.h
*** src/include/utils/timestamp.h 13 Jul 2006 16:49:20 -0000 1.62
--- src/include/utils/timestamp.h 1 Sep 2006 12:28:24 -0000
***************
*** 161,171 ****
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))
#define INTERVAL_MASK(b) (1 << (b))
--- 161,175 ----
typedef double fsec_t;
! #endif
!
! /*
! * Round off to MAX_TIMESTAMP_PRECISION decimal places.
! * This is also used for rounding off intervals, and
! * for rounding interval multiplication/division calculations.
! */
#define TS_PREC_INV 1000000.0
#define TSROUND(j) (rint(((double) (j)) * TS_PREC_INV) / TS_PREC_INV)
#define TIMESTAMP_MASK(b) (1 << (b))
#define INTERVAL_MASK(b) (1 << (b))