Re: [PATCHES] [HACKERS] Interval aggregate regression failure
On Sep 5, 2006, at 10:14 , Bruce Momjian wrote: Bruce Momjian wrote: OK, updated patch. It will fix the =24:00:00 case because it cascades up if the remainder number of seconds is greater or equal to one day. One open item is that it still might show 24 hours if the seconds computation combined with the remaning seconds 24 hours. Not sure if that should be handled or not. If you fix that, you really are cascading up because the resulting seconds might be less than the computed value, e.g. result is 23:00:00, remainder is 02:00:00, cascade up would be 1 day, 01:00:00. I am unsure we want to do that. Right now, this will show 25:00:00. Updated patch that uses TSROUND for partial month and days. Michael says the tests look good on his system. I think we are done with this bug. :-) Please find attached regression tests for this patch. Michael Glaesemann grzm seespotcode net 17interval_muldiv_0905T2053+0900.diff Description: Binary data ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] [HACKERS] Interval aggregate regression failure
Patch applied. Thanks. --- Michael Glaesemann wrote: On Sep 5, 2006, at 10:14 , Bruce Momjian wrote: Bruce Momjian wrote: OK, updated patch. It will fix the =24:00:00 case because it cascades up if the remainder number of seconds is greater or equal to one day. One open item is that it still might show 24 hours if the seconds computation combined with the remaning seconds 24 hours. Not sure if that should be handled or not. If you fix that, you really are cascading up because the resulting seconds might be less than the computed value, e.g. result is 23:00:00, remainder is 02:00:00, cascade up would be 1 day, 01:00:00. I am unsure we want to do that. Right now, this will show 25:00:00. Updated patch that uses TSROUND for partial month and days. Michael says the tests look good on his system. I think we are done with this bug. :-) Please find attached regression tests for this patch. Michael Glaesemann grzm seespotcode net [ Attachment, skipping... ] -- Bruce Momjian [EMAIL PROTECTED] EnterpriseDBhttp://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] [HACKERS] Interval aggregate regression failure (expected seems
On Sep 3, 2006, at 12:34 , Bruce Momjian wrote: OK, I worked with Michael and I think this is the best we are going to do to fix this. It has one TSROUND call for Powerpc, and that is documented. Applied. As I was working up regression tests, I found a case that this patch doesn't handle. select interval '4 mon' * .3 as product_h; product_h --- 1 mon 5 days 24:00:00 (1 row) This should be 1 mon 6 days. It fails for any number of months greater than 3 that is not evenly divisible by 10, greater than 3 months. Do we need to look at the month remainder separately? Michael Glaesemann grzm seespotcode net ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] [HACKERS] Interval aggregate regression failure
Is this non-datetime integer only or both? I cannot reproduce the failure here. --- Michael Glaesemann wrote: On Sep 3, 2006, at 12:34 , Bruce Momjian wrote: OK, I worked with Michael and I think this is the best we are going to do to fix this. It has one TSROUND call for Powerpc, and that is documented. Applied. As I was working up regression tests, I found a case that this patch doesn't handle. select interval '4 mon' * .3 as product_h; product_h --- 1 mon 5 days 24:00:00 (1 row) This should be 1 mon 6 days. It fails for any number of months greater than 3 that is not evenly divisible by 10, greater than 3 months. Do we need to look at the month remainder separately? Michael Glaesemann grzm seespotcode net -- Bruce Momjian [EMAIL PROTECTED] EnterpriseDBhttp://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] [HACKERS] Interval aggregate regression failure (expected seems
Bruce Momjian [EMAIL PROTECTED] writes: Is this non-datetime integer only or both? I cannot reproduce the failure here. On HPPA with float datetimes with today's code, Michael's case works but it took me less than two minutes to find one that doesn't: regression=# select interval '14 mon' * 8.2 as product_h; product_h - 9 years 6 mons 23 days 24:00:00 (1 row) I reiterate my comment that this approach will never work; any small amount of experimentation will turn up cases that don't round correctly on one platform or another. Float arithmetic is inherently inexact. regards, tom lane ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] [HACKERS] Interval aggregate regression failure
Michael Glaesemann wrote: On Sep 3, 2006, at 12:34 , Bruce Momjian wrote: OK, I worked with Michael and I think this is the best we are going to do to fix this. It has one TSROUND call for Powerpc, and that is documented. Applied. As I was working up regression tests, I found a case that this patch doesn't handle. select interval '4 mon' * .3 as product_h; product_h --- 1 mon 5 days 24:00:00 (1 row) This should be 1 mon 6 days. It fails for any number of months greater than 3 that is not evenly divisible by 10, greater than 3 months. Do we need to look at the month remainder separately? Another question. Is this result correct? test= select '999 months 999 days'::interval / 100; ?column? - 9 mons 38 days 40:33:36 (1 row) Should that be: 9 mons 39 days 16:33:36 The core problem is that the combined remainder seconds of months and days is 24 hours. -- Bruce Momjian [EMAIL PROTECTED] EnterpriseDBhttp://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] [HACKERS] Interval aggregate regression failure (expected seems
On Sep 4, 2006, at 4:45 , Bruce Momjian wrote: Another question. Is this result correct? test= select '999 months 999 days'::interval / 100; ?column? - 9 mons 38 days 40:33:36 (1 row) Should that be: 9 mons 39 days 16:33:36 Yeah, I think it should be. I had been thinking of treating the month and day component as having separate time contributions, but it makes more sense to think of month as a collection of days first, integral or no, and then cascade down the fractional portion of the combined days component to time. I.e., 9.99 mon is 9 mon 29.7 days, rather than 9 mon 29 days 60480 sec. Michael Glaesemann grzm seespotcode net ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] [HACKERS] Interval aggregate regression failure
Michael Glaesemann wrote: On Sep 4, 2006, at 4:45 , Bruce Momjian wrote: Another question. Is this result correct? test= select '999 months 999 days'::interval / 100; ?column? - 9 mons 38 days 40:33:36 (1 row) Should that be: 9 mons 39 days 16:33:36 Yeah, I think it should be. I had been thinking of treating the month and day component as having separate time contributions, but it makes more sense to think of month as a collection of days first, integral or no, and then cascade down the fractional portion of the combined days component to time. I.e., 9.99 mon is 9 mon 29.7 days, rather than 9 mon 29 days 60480 sec. No, I don't think so. If we do that, there is no reason to cascade at all. Why not just say 9.1 months? I am going to work on a patch to fix the 24 hours case, which will fix your 24:00:00 case at the same time. Will post in an hour. -- Bruce Momjian [EMAIL PROTECTED] EnterpriseDBhttp://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] [HACKERS] Interval aggregate regression failure
Tom Lane wrote: Bruce Momjian [EMAIL PROTECTED] writes: Is this non-datetime integer only or both? I cannot reproduce the failure here. On HPPA with float datetimes with today's code, Michael's case works but it took me less than two minutes to find one that doesn't: regression=# select interval '14 mon' * 8.2 as product_h; product_h - 9 years 6 mons 23 days 24:00:00 (1 row) I reiterate my comment that this approach will never work; any small amount of experimentation will turn up cases that don't round correctly on one platform or another. Float arithmetic is inherently inexact. Working on a new patch to round; will post. -- Bruce Momjian [EMAIL PROTECTED] EnterpriseDBhttp://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] [HACKERS] Interval aggregate regression failure
Bruce Momjian wrote:
Michael Glaesemann wrote:
On Sep 3, 2006, at 12:34 , Bruce Momjian wrote:
OK, I worked with Michael and I think this is the best we are going to
do to fix this. It has one TSROUND call for Powerpc, and that is
documented. Applied.
As I was working up regression tests, I found a case that this patch
doesn't handle.
select interval '4 mon' * .3 as product_h;
product_h
---
1 mon 5 days 24:00:00
(1 row)
This should be 1 mon 6 days. It fails for any number of months
greater than 3 that is not evenly divisible by 10, greater than 3
months. Do we need to look at the month remainder separately?
Another question. Is this result correct?
test= select '999 months 999 days'::interval / 100;
?column?
-
9 mons 38 days 40:33:36
(1 row)
Should that be:
9 mons 39 days 16:33:36
The core problem is that the combined remainder seconds of months and
days is 24 hours.
OK, updated patch. It will fix the =24:00:00 case because it cascades
up if the remainder number of seconds is greater or equal to one day.
One open item is that it still might show 24 hours if the seconds
computation combined with the remaning seconds 24 hours. Not sure if
that should be handled or not. If you fix that, you really are
cascading up because the resulting seconds might be less than the
computed value, e.g. result is 23:00:00, remainder is 02:00:00, cascade
up would be 1 day, 01:00:00. I am unsure we want to do that. Right
now, this will show 25:00:00.
--
Bruce Momjian [EMAIL PROTECTED]
EnterpriseDBhttp://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Index: src/backend/utils/adt/timestamp.c
===
RCS file: /cvsroot/pgsql/src/backend/utils/adt/timestamp.c,v
retrieving revision 1.166
diff -c -c -r1.166 timestamp.c
*** src/backend/utils/adt/timestamp.c 3 Sep 2006 03:34:04 - 1.166
--- src/backend/utils/adt/timestamp.c 4 Sep 2006 03:49:21 -
***
*** 2525,2541
sec_remainder = (orig_day * (double)SECS_PER_DAY) * factor -
result-day * (double)SECS_PER_DAY +
(month_remainder_days - (int32) month_remainder_days) * SECS_PER_DAY;
/* cascade units down */
result-day += (int32) month_remainder_days;
#ifdef HAVE_INT64_TIMESTAMP
result-time = rint(span-time * factor + sec_remainder * USECS_PER_SEC);
#else
! /*
! * TSROUND() needed to prevent -146:23:60.00 output on PowerPC for
! * SELECT interval '-41 mon -12 days -360:00' * 0.3;
! */
! result-time = span-time * factor + TSROUND(sec_remainder);
#endif
PG_RETURN_INTERVAL_P(result);
--- 2524,2547
sec_remainder = (orig_day * (double)SECS_PER_DAY) * factor -
result-day * (double)SECS_PER_DAY +
(month_remainder_days - (int32) month_remainder_days) * SECS_PER_DAY;
+ /*
+ * Might have 24:00:00 hours due to rounding, or 24 hours because of
+ * time cascade from months and days. It might still be 24 if the
+ * combination of cascade and the seconds factor operation itself.
+ */
+ sec_remainder = TSROUND(sec_remainder);
+ if (Abs(sec_remainder) = SECS_PER_DAY)
+ {
+ sec_remainder -= (int)(sec_remainder / SECS_PER_DAY) * SECS_PER_DAY;
+ result-day += (int)(sec_remainder / SECS_PER_DAY);
+ }
/* cascade units down */
result-day += (int32) month_remainder_days;
#ifdef HAVE_INT64_TIMESTAMP
result-time = rint(span-time * factor + sec_remainder * USECS_PER_SEC);
#else
! result-time = span-time * factor + sec_remainder;
#endif
PG_RETURN_INTERVAL_P(result);
***
*** 2579,2584
--- 2585,2596
sec_remainder = (orig_day * (double)SECS_PER_DAY) / factor -
result-day * (double)SECS_PER_DAY +
(month_remainder_days - (int32) month_remainder_days) * SECS_PER_DAY;
+ sec_remainder = TSROUND(sec_remainder);
+ if (Abs(sec_remainder) = SECS_PER_DAY)
+ {
+ sec_remainder -= (int)(sec_remainder / SECS_PER_DAY) * SECS_PER_DAY;
+ result-day += (int)(sec_remainder / SECS_PER_DAY);
+ }
/* cascade units down */
result-day += (int32) month_remainder_days;
***
*** 2586,2592
result-time = rint(span-time / factor + sec_remainder * USECS_PER_SEC);
#else
/* See TSROUND comment in interval_mul(). */
! result-time = span-time / factor + TSROUND(sec_remainder);
#endif
PG_RETURN_INTERVAL_P(result);
--- 2598,2604
result-time = rint(span-time / factor + sec_remainder * USECS_PER_SEC);
#else
/* See TSROUND comment in interval_mul(). */
! result-time = span-time / factor + sec_remainder;
#endif
PG_RETURN_INTERVAL_P(result);
Index: src/include/utils/timestamp.h
===
RCS file: /cvsroot/pgsql/src/include/utils/timestamp.h,v
retrieving revision 1.62
diff -c -c -r1.62
Re: [PATCHES] [HACKERS] Interval aggregate regression failure
On Sep 1, 2006, at 11:31 , Bruce Momjian wrote: Tom Lane wrote: Bruce Momjian [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] [HACKERS] Interval aggregate regression failure
On Sep 1, 2006, at 11:03 , Bruce Momjian wrote: 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? I think the 23:60 is a time rounding issue that isn't covered in this patch. I am not against fixing it, but does the submitted patch improve things or not? Given we are post-feature freeze, we don't have time to fix all the interval issues. Your patch doesn't fix the things Tom referenced (nor did you intend it to). I just wanted to to collect examples of all the known issues with the interval code in one place. Probably too ambitious for September 1. Is it worth looking into the overflow and subtraction issues for 8.2? It seems to me they're bugs rather than features. Or are these 8.3 since it's so late? Michael Glaesemann grzm seespotcode net ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] [HACKERS] Interval aggregate regression failure
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 - 1.165 --- src/backend/utils/adt/timestamp.c 1 Sep 2006 11:26:12 - *** *** 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
Re: [PATCHES] [HACKERS] Interval aggregate regression failure
Please ignore the patch I just sent. Much too quick with the send button. Michael Glaesemann grzm seespotcode net ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] [HACKERS] Interval aggregate regression failure
Michael Glaesemann [EMAIL PROTECTED] writes: Is it worth looking into the overflow and subtraction issues for 8.2? It seems to me they're bugs rather than features. Or are these 8.3 since it's so late? IMHO they're bugs not new features, and therefore perfectly fair game to work on during beta. But for the moment I'd suggest staying focused on the interval_mul/interval_div roundoff issue. regards, tom lane ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] [HACKERS] Interval aggregate regression failure
Bruce Momjian [EMAIL PROTECTED] writes: Well, the patch only multiplies by 30, so the interval would have to span +5 million years to overflow. I don't see any reason to add rounding until we get an actual query that needs it Have you tried your patch against the various cases that have been discussed in the past? In particular there were several distinct examples of this behavior posted at the beginning of the thread, and I'd not assume that a fix for one handles them all. BTW, while trolling for examples I came across this: http://archives.postgresql.org/pgsql-bugs/2005-10/msg00307.php pointing out some issues that still haven't been addressed. regards, tom lane ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] [HACKERS] Interval aggregate regression failure
Tom Lane wrote: Bruce Momjian [EMAIL PROTECTED] writes: OK, here is a much nicer patch. The fix is to do no rounding, but to find the number of days before applying the factor adjustment. You have forgotten the problem of the factor not being exactly representable (eg, things like '10 days' * 0.1 not giving the expected result). Also, as coded this is subject to integer-overflow risks that weren't there before. That could be fixed, but it's still only addressing a subset of the problems. I don't think you can fix them all without rounding somewhere. Well, the patch only multiplies by 30, so the interval would have to span +5 million years to overflow. I don't see any reason to add rounding until we get an actual query that needs it (and because rounding is arbitrary). I think the proposed fix is the best we can do at this time. -- Bruce Momjian [EMAIL PROTECTED] EnterpriseDBhttp://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] [HACKERS] Interval aggregate regression failure
Tom Lane wrote: Bruce Momjian [EMAIL PROTECTED] writes: Well, the patch only multiplies by 30, so the interval would have to span +5 million years to overflow. I don't see any reason to add rounding until we get an actual query that needs it Have you tried your patch against the various cases that have been discussed in the past? In particular there were several distinct examples of this behavior posted at the beginning of the thread, and I'd not assume that a fix for one handles them all. Yes, it fixes all posted examples, except one that displays 23:60. I cannot reproduce that failure from Powerpc so am waiting for Michael to test it. BTW, while trolling for examples I came across this: http://archives.postgresql.org/pgsql-bugs/2005-10/msg00307.php pointing out some issues that still haven't been addressed. Yea, that is a bunch of issues. They are already on the TODO list. -- Bruce Momjian [EMAIL PROTECTED] EnterpriseDBhttp://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] [HACKERS] Interval aggregate regression failure
On Sep 1, 2006, at 5:05 , Bruce Momjian wrote: Tom Lane wrote: Bruce Momjian [EMAIL PROTECTED] writes: Well, the patch only multiplies by 30, so the interval would have to span +5 million years to overflow. I don't see any reason to add rounding until we get an actual query that needs it Have you tried your patch against the various cases that have been discussed in the past? In particular there were several distinct examples of this behavior posted at the beginning of the thread, and I'd not assume that a fix for one handles them all. Yes, it fixes all posted examples, except one that displays 23:60. I cannot reproduce that failure from Powerpc so am waiting for Michael to test it. Here's your patch tested on my machine, both with and without -- enable-integer-datetimes. I've tweaked the ad hoc test suite to include a case where the days and time differ in sign and added a couple of queries to the ad hoc test suite to include the problems Tom referred to--not that this patch will fix them, but to keep the known problems together. I hope to add more to this to test more edge cases. Unfortunately the problem still occur (see product_d), and --enable- integer-datetimes is pretty broken with this patch. Michael Glaesemann grzm seespotcode net -- test queries 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; select interval '41 mon 12 days 360:00' / 10 as quotient_a , interval '-41 mon -12 days +360:00' / 10 as quotient_b , interval '-41 mon 12 days 360:00' / 10 as quotient_c , interval '-41 mon -12 days -360:00' / 10 as quotient_d; select interval '-12 days' * 0.3; select 1 * '100 hours'::interval as ten billion; set time zone 'EST5EDT'; select '2005-10-29 13:22:00-04'::timestamptz + '1 day'::interval as 2005-01-30 13:22:00-05; select '2005-10-30 13:22:00-05'::timestamptz - '2005-10-29 13:22:00-04'::timestamptz as a day; set time zone local; -- end test queries -- without --enable-integer-datetimes 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 (1 row) select interval '41 mon 12 days 360:00' / 10 as quotient_a , interval '-41 mon -12 days +360:00' / 10 as quotient_b , interval '-41 mon 12 days 360:00' / 10 as quotient_c , interval '-41 mon -12 days -360:00' / 10 as quotient_d; quotient_a |quotient_b | quotient_c |quotient_d +--- +---+--- 4 mons 4 days 40:48:00 | -4 mons -4 days +31:12:00 | -4 mons -2 days +40:48:00 | -4 mons -4 days -40:48:00 (1 row) select interval '-12 days' * 0.3; ?column? -- -3 days -14:23:60.00 (1 row) select 1 * '100 hours'::interval as ten billion; ten billion -- 2147483647:00:00 (1 row) set time zone 'EST5EDT'; SET select '2005-10-29 13:22:00-04'::timestamptz + '1 day'::interval as 2005-01-30 13:22:00-05; 2005-01-30 13:22:00-05 2005-10-30 13:22:00-05 (1 row) select '2005-10-30 13:22:00-05'::timestamptz - '2005-10-29 13:22:00-04'::timestamptz as a day; a day 1 day 01:00:00 (1 row) set time zone local; SET -- with --enable-integer-datetimes 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:24:00 (1 row) select interval '41 mon 12 days 360:00' / 10 as quotient_a , interval '-41 mon -12 days +360:00' / 10 as quotient_b , interval '-41 mon 12 days 360:00' / 10 as quotient_c , interval '-41 mon -12 days -360:00' / 10 as quotient_d; quotient_a |quotient_b | quotient_c |quotient_d +---
Re: [PATCHES] [HACKERS] Interval aggregate regression failure
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? I think the 23:60 is a time rounding issue that isn't covered in this patch. I am not against fixing it, but does the submitted patch improve things or not? Given we are post-feature freeze, we don't have time to fix all the interval issues. --- Michael Glaesemann wrote: On Sep 1, 2006, at 5:05 , Bruce Momjian wrote: Tom Lane wrote: Bruce Momjian [EMAIL PROTECTED] writes: Well, the patch only multiplies by 30, so the interval would have to span +5 million years to overflow. I don't see any reason to add rounding until we get an actual query that needs it Have you tried your patch against the various cases that have been discussed in the past? In particular there were several distinct examples of this behavior posted at the beginning of the thread, and I'd not assume that a fix for one handles them all. Yes, it fixes all posted examples, except one that displays 23:60. I cannot reproduce that failure from Powerpc so am waiting for Michael to test it. Here's your patch tested on my machine, both with and without -- enable-integer-datetimes. I've tweaked the ad hoc test suite to include a case where the days and time differ in sign and added a couple of queries to the ad hoc test suite to include the problems Tom referred to--not that this patch will fix them, but to keep the known problems together. I hope to add more to this to test more edge cases. -- Bruce Momjian [EMAIL PROTECTED] EnterpriseDBhttp://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] [HACKERS] Interval aggregate regression failure
Tom Lane wrote: Bruce Momjian [EMAIL PROTECTED] 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 [EMAIL PROTECTED] EnterpriseDBhttp://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] [HACKERS] Interval aggregate regression failure (expected seems
On Aug 30, 2006, at 12:50 , Bruce Momjian wrote: Yea, I see that -122:23:60.00. After applying your patch, I believe that on my machine it's the contribution from the day component that is producing the 23:60.00. For example, select interval '-12 days' * 0.3; ?column? -- -3 days -14:23:60.00 (1 row) I think some kind of rounding needs to be done to the day contribution as well. I'm continuing to work on it, but wanted to get this out there. Michael Glaesemann grzm seespotcode net ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] [HACKERS] Interval aggregate regression failure
Michael Glaesemann wrote: Yea, just an optimization, but I was worried that the computations might throw problems for certain numbers, so I figured I would only trigger it when necessary. Thanks for the explanation. Helps me know I might actually be learning this. Patch attached. It also fixes a regression test output too. Thanks for the patch. I'll look at it more closely tonight. OK, here is a much nicer patch. The fix is to do no rounding, but to find the number of days before applying the factor adjustment. It passes all your tests here: test= select '41 mon 10:00:00'::interval / 10 as pos; pos 4 mons 3 days 01:00:00 (1 row) test= 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 12 days 122:24:00 | 1 year 6 days -122:24:00 | -1 years -6 days +122:24:00 | -1 years -12 days -122:24:00 (1 row) test= select interval '41 mon 12 days 360:00' / 10 as quotient_a , interval '41 mon -12 days -360:00' / 10 as quotient_b , interval '-41 mon 12 days 360:00' / 10 as quotient_c , interval '-41 mon -12 days -360:00' / 10 as quotient_d; quotient_a | quotient_b|quotient_c |quotient_d +-+---+--- 4 mons 4 days 40:48:00 | 4 mons 2 days -40:48:00 | -4 mons -2 days +40:48:00 | -4 mons -4 days -40:48:00 (1 row) What do you see on your platform? -- Bruce Momjian [EMAIL PROTECTED] EnterpriseDBhttp://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + Index: src/backend/utils/adt/timestamp.c === RCS file: /cvsroot/pgsql/src/backend/utils/adt/timestamp.c,v retrieving revision 1.165 diff -c -c -r1.165 timestamp.c *** src/backend/utils/adt/timestamp.c 13 Jul 2006 16:49:16 - 1.165 --- src/backend/utils/adt/timestamp.c 30 Aug 2006 17:08:12 - *** *** 2496,2501 --- 2496,2502 day_remainder, month_remainder_days; Interval *result; + int32 orig_month = span-month; result = (Interval *) palloc(sizeof(Interval)); *** *** 2516,2523 * using justify_hours and/or justify_days. */ ! /* 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; --- 2517,2533 * using justify_hours and/or justify_days. */ ! /* ! * Fractional months full days into days. ! * ! * The remainders suffer from float rounding, so instead of ! * doing the computation using just the remainder, we calculate ! * the total number of days and subtract. Specifically, we are ! * multipling by DAYS_PER_MONTH before dividing by factor. ! * This greatly reduces rounding errors. ! */ ! month_remainder_days = (orig_month * DAYS_PER_MONTH) * factor - ! result-month * DAYS_PER_MONTH; result-day += (int32) month_remainder_days; /* fractional months partial days into time */ day_remainder += month_remainder_days - (int32) month_remainder_days; *** *** 2550,2556 day_remainder, month_remainder_days; Interval *result; ! result = (Interval *) palloc(sizeof(Interval)); if (factor == 0.0) --- 2560,2567 day_remainder, month_remainder_days; Interval *result; ! int32 orig_month = span-month; ! result = (Interval *) palloc(sizeof(Interval)); if (factor == 0.0) *** *** 2566,2576 day_remainder -= result-day; /* ! * Handle any fractional parts the same way as in interval_mul. */ ! ! /* 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; --- 2577,2587 day_remainder -= result-day; /* ! * Fractional months full days into days. See comment in ! * interval_mul(). */ ! month_remainder_days = (orig_month * DAYS_PER_MONTH) / factor - !
Re: [PATCHES] [HACKERS] Interval aggregate regression failure
Michael Glaesemann wrote: On Aug 30, 2006, at 12:50 , Bruce Momjian wrote: Yea, I see that -122:23:60.00. After applying your patch, I believe that on my machine it's the contribution from the day component that is producing the 23:60.00. For example, select interval '-12 days' * 0.3; ?column? -- -3 days -14:23:60.00 (1 row) I think some kind of rounding needs to be done to the day contribution as well. I'm continuing to work on it, but wanted to get this out there. Please test with my new patch and let me know how it looks. Thanks. -- Bruce Momjian [EMAIL PROTECTED] EnterpriseDBhttp://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] [HACKERS] Interval aggregate regression failure (expected seems
I think I've got it. I plan to update the regression tests this
evening, but I wanted to post what I believe is a solution.
select '41 mon'::interval / 10;
?column?
---
4 mons 3 days
(1 row)
select '41 mon 360:00'::interval / 10 as pos
, '-41 mon -360:00'::interval / 10 as neg;
pos |neg
+---
4 mons 3 days 36:00:00 | -4 mons -3 days -36:00:00
(1 row)
select '41 mon -360:00'::interval / 10 as pos
, '-41 mon 360:00'::interval / 10 as neg;
pos |neg
-+---
4 mons 3 days -36:00:00 | -4 mons -3 days +36:00:00
(1 row)
If anyone sees anything untoward, please let me know and I'll do my
best to fix it. Also, should the duplicate code in interval_mul and
interval_div be refactored into its own function?
Thanks!
Michael Glaesemann
grzm seespotcode net
---8-
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 - 1.165
--- src/backend/utils/adt/timestamp.c 29 Aug 2006 06:20:03 -
***
*** 2494,2500
float8 factor = PG_GETARG_FLOAT8(1);
double month_remainder,
day_remainder,
! month_remainder_days;
Interval *result;
result = (Interval *) palloc(sizeof(Interval));
--- 2494,2502
float8 factor = PG_GETARG_FLOAT8(1);
double month_remainder,
day_remainder,
! month_remainder_days,
! month_remainder_day_frac,
! month_remainder_time;
Interval *result;
result = (Interval *) palloc(sizeof(Interval));
***
*** 2519,2526
/* 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);
--- 2521,2556
/* fractional months full days into days */
month_remainder_days = month_remainder * DAYS_PER_MONTH;
result-day += (int32) month_remainder_days;
!
! month_remainder_day_frac = month_remainder_days - (int32)
month_remainder_days;
!
! #ifdef HAVE_INT64_TIMESTAMP
! month_remainder_day_frac = month_remainder_days - (int32)
month_remainder_days;
! month_remainder_time = month_remainder_day_frac * USECS_PER_DAY;
! if (rint(month_remainder_time) == USECS_PER_DAY)
! {
! result-day++;
! }
! else if ((rint(month_remainder_time)) == -USECS_PER_DAY)
! {
! result-day--;
! }
! #else
! month_remainder_time = month_remainder_day_frac * SECS_PER_DAY;
! if ((TSROUND(month_remainder_time) == SECS_PER_DAY))
! {
! result-day++;
! }
! else if ((TSROUND(month_remainder_time) == -SECS_PER_DAY))
! {
! result-day--;
! }
! #endif
! else
! {
! /* fractional months partial days into time */
! day_remainder += month_remainder_day_frac;
! }
#ifdef HAVE_INT64_TIMESTAMP
result-time = rint(span-time * factor + day_remainder *
USECS_PER_DAY);
***
*** 2548,2558
float8 factor = PG_GETARG_FLOAT8(1);
double month_remainder,
day_remainder,
! month_remainder_days;
Interval *result;
result = (Interval *) palloc(sizeof(Interval));
if (factor == 0.0)
ereport(ERROR,
(errcode(ERRCODE_DIVISION_BY_ZERO),
--- 2578,2596
float8 factor = PG_GETARG_FLOAT8(1);
double month_remainder,
day_remainder,
! month_remainder_days,
! month_remainder_day_frac,
! month_remainder_time;
Interval *result;
result = (Interval *) palloc(sizeof(Interval));
+ /*
+ a: (fl) select '41 mon'::interval / 10;
+ *span = { time = 0., day = 0, month = 41 }
+ factor = 10.
+ */
+
if (factor == 0.0)
ereport(ERROR,
(errcode(ERRCODE_DIVISION_BY_ZERO),
***
*** 2560,2579
month_remainder = span-month / factor;
day_remainder = span-day /
Re: [PATCHES] [HACKERS] Interval aggregate regression failure
Michael Glaesemann wrote: On Aug 29, 2006, at 15:38 , Michael Glaesemann wrote: I think I've got it. I plan to update the regression tests this evening, but I wanted to post what I believe is a solution. I've cleaned up the patch a bit in terms of whitespace, comments, and parens. I've also updated the interval and horology regression tests. The horology tests needed updating because I added 5 rows to INTERVAL_TBL. I didn't check the math for every row of time(tz | stamp | stamptz)/interval arithmetic in the horology tests as I think problems in this area would have shown up before. Does that make sense or it just rationalization on my part? Both with and without --enable-integer-datetimes pass the regression tests. Uh, I came up with a cleaner one, I think. I didn't test --enable-integer-datetimes yet. I tested a few of your examples: test= select '41 mon 10:00:00'::interval / 10 as pos; pos 4 mons 3 days 01:00:00 (1 row) It basically rounds the remainders to full values if they are close to full (+/- 0.01). -- Bruce Momjian [EMAIL PROTECTED] EnterpriseDBhttp://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + Index: src/backend/utils/adt/timestamp.c === RCS file: /cvsroot/pgsql/src/backend/utils/adt/timestamp.c,v retrieving revision 1.165 diff -c -c -r1.165 timestamp.c *** src/backend/utils/adt/timestamp.c 13 Jul 2006 16:49:16 - 1.165 --- src/backend/utils/adt/timestamp.c 29 Aug 2006 16:06:49 - *** *** 2505,2510 --- 2505,2513 result-day = (int32) day_remainder; month_remainder -= result-month; day_remainder -= result-day; + if (day_remainder != (int32)day_remainder + TSROUND(day_remainder) == rint(day_remainder)) + day_remainder = rint(day_remainder); /* * The above correctly handles the whole-number part of the month and day *** *** 2518,2523 --- 2521,2529 /* fractional months full days into days */ month_remainder_days = month_remainder * DAYS_PER_MONTH; + if (month_remainder_days != (int32)month_remainder_days + TSROUND(month_remainder_days) == rint(month_remainder_days)) + month_remainder_days = rint(month_remainder_days); result-day += (int32) month_remainder_days; /* fractional months partial days into time */ day_remainder += month_remainder_days - (int32) month_remainder_days; *** *** 2564,2569 --- 2570,2578 result-day = (int32) day_remainder; month_remainder -= result-month; day_remainder -= result-day; + if (day_remainder != (int32)day_remainder + TSROUND(day_remainder) == rint(day_remainder)) + day_remainder = rint(day_remainder); /* * Handle any fractional parts the same way as in interval_mul. *** *** 2571,2576 --- 2580,2588 /* fractional months full days into days */ month_remainder_days = month_remainder * DAYS_PER_MONTH; + if (month_remainder_days != (int32)month_remainder_days + TSROUND(month_remainder_days) == rint(month_remainder_days)) + month_remainder_days = rint(month_remainder_days); result-day += (int32) month_remainder_days; /* fractional months partial days into time */ day_remainder += month_remainder_days - (int32) month_remainder_days; ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] [HACKERS] Interval aggregate regression failure (expected seems
On Aug 30, 2006, at 1:13 , Bruce Momjian wrote: Uh, I came up with a cleaner one, I think. I didn't test --enable-integer-datetimes yet. Cool. It's indeed much cleaner. Thanks, Bruce. I'm about to head to bed, but I'll look at it more closely tomorrow. I also noticed that my regression tests didn't exercise the code I thought it did. If you have a chance before I get to it, you might want to try these as well: select interval '41 mon 12 days 360:00' / 10 as quotient_a , interval '41 mon -12 days -360:00' / 10 as quotient_b , interval '-41 mon 12 days 360:00' / 10 as quotient_c , interval '-41 mon -12 days -360:00' / 10 as quotient_d; quotient_a | quotient_b| quotient_c |quotient_d +- +---+--- 4 mons 4 days 40:48:00 | 4 mons 2 days -40:48:00 | -4 mons -2 days +40:48:00 | -4 mons -4 days -40:48:00 (1 row) 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 12 days 122:24:00 | 1 year 6 days -122:23:60.00 | -1 years -6 days +122:24:00 | -1 years -12 days -122:23:60.00 (1 row) The quotients look fine, but I'm wondering if another set of rounding is needed to bump those -122:23:60.00 to -122:24:00 in product_b and product_d. Michael Glaesemann grzm seespotcode net ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] [HACKERS] Interval aggregate regression failure
Michael Glaesemann wrote: On Aug 30, 2006, at 1:13 , Bruce Momjian wrote: Uh, I came up with a cleaner one, I think. I didn't test --enable-integer-datetimes yet. Cool. It's indeed much cleaner. Thanks, Bruce. I'm about to head to bed, but I'll look at it more closely tomorrow. I also noticed that my regression tests didn't exercise the code I thought it did. If you have a chance before I get to it, you might want to try these as well: select interval '41 mon 12 days 360:00' / 10 as quotient_a , interval '41 mon -12 days -360:00' / 10 as quotient_b , interval '-41 mon 12 days 360:00' / 10 as quotient_c , interval '-41 mon -12 days -360:00' / 10 as quotient_d; quotient_a | quotient_b| quotient_c |quotient_d +- +---+--- 4 mons 4 days 40:48:00 | 4 mons 2 days -40:48:00 | -4 mons -2 days +40:48:00 | -4 mons -4 days -40:48:00 (1 row) 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 12 days 122:24:00 | 1 year 6 days -122:23:60.00 | -1 years -6 days +122:24:00 | -1 years -12 days -122:23:60.00 (1 row) The quotients look fine, but I'm wondering if another set of rounding is needed to bump those -122:23:60.00 to -122:24:00 in product_b and product_d. Here are the results using my newest patch: test= select interval '41 mon 12 days 360:00' / 10 as quotient_a , interval '41 mon -12 days -360:00' / 10 as quotient_b , interval '-41 mon 12 days 360:00' / 10 as quotient_c , interval '-41 mon -12 days -360:00' / 10 as quotient_d; quotient_a | quotient_b|quotient_c |quotient_d +-+---+--- 4 mons 4 days 40:48:00 | 4 mons 2 days -40:48:00 | -4 mons -2 days +40:48:00 | -4 mons -4 days -40:48:00 (1 row) test= 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 12 days 122:24:00 | 1 year 6 days -122:24:00 | -1 years -6 days +122:24:00 | -1 years -12 days -122:24:00 (1 row) I see no 23:60 entries. I realize the problem with my first patch. I was rounding at the 'seconds' level, but that is too late in the process. The rounding has to happen right after the division. In fact the only rounding problem I can find is with month_remainder_days, because of a division by factor, and a multiplication to convert it to days. The combination of steps is where the rounding problem is happening. The patch is even smaller now. The code assume if it is within 0.01 of a whole number, it should be rounded to a whole number. Patch attached with comments added. -- Bruce Momjian [EMAIL PROTECTED] EnterpriseDBhttp://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + Index: src/backend/utils/adt/timestamp.c === RCS file: /cvsroot/pgsql/src/backend/utils/adt/timestamp.c,v retrieving revision 1.165 diff -c -c -r1.165 timestamp.c *** src/backend/utils/adt/timestamp.c 13 Jul 2006 16:49:16 - 1.165 --- src/backend/utils/adt/timestamp.c 29 Aug 2006 22:04:41 - *** *** 2518,2523 --- 2518,2530 /* 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 0.01 of an integer, we round them to integers. +*/ + if (month_remainder_days != (int32)month_remainder_days + TSROUND(month_remainder_days) == rint(month_remainder_days)) + month_remainder_days = rint(month_remainder_days); result-day += (int32) month_remainder_days; /* fractional months partial days into time */ day_remainder += month_remainder_days -
Re: [PATCHES] [HACKERS] Interval aggregate regression failure (expected seems
On Aug 30, 2006, at 7:12 , Bruce Momjian wrote: Here are the results using my newest patch: test= select interval '41 mon 12 days 360:00' / 10 as quotient_a , interval '41 mon -12 days -360:00' / 10 as quotient_b , interval '-41 mon 12 days 360:00' / 10 as quotient_c , interval '-41 mon -12 days -360:00' / 10 as quotient_d; quotient_a | quotient_b| quotient_c |quotient_d +- +---+--- 4 mons 4 days 40:48:00 | 4 mons 2 days -40:48:00 | -4 mons -2 days +40:48:00 | -4 mons -4 days -40:48:00 (1 row) test= 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 12 days 122:24:00 | 1 year 6 days -122:24:00 | -1 years -6 days +122:24:00 | -1 years -12 days -122:24:00 (1 row) I see no 23:60 entries. Using Bruce's newest patch, I still get the 23:60 entries on my machine (no integer-datetimes) select version(); version - PostgreSQL 8.2devel on powerpc-apple-darwin8.7.0, compiled by GCC powerpc-apple-darwin8-gcc-4.0.1 (GCC) 4.0.1 (Apple Computer, Inc. build 5341) (1 row) select interval '41 mon 12 days 360:00' / 10 as quotient_a , interval '41 mon -12 days -360:00' / 10 as quotient_b , interval '-41 mon 12 days 360:00' / 10 as quotient_c , interval '-41 mon -12 days -360:00' / 10 as quotient_d; quotient_a | quotient_b| quotient_c |quotient_d +- +---+--- 4 mons 4 days 40:48:00 | 4 mons 2 days -40:48:00 | -4 mons -2 days +40:48:00 | -4 mons -4 days -40:48:00 (1 row) 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 12 days 122:24:00 | 1 year 6 days -122:23:60.00 | -1 years -6 days +122:24:00 | -1 years -12 days -122:23:60.00 (1 row) The code assume if it is within 0.01 of a whole number, it should be rounded to a whole number. Patch attached with comments added. /* 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 0.01 of an integer, we round them to integers. +*/ + if (month_remainder_days != (int32)month_remainder_days + TSROUND(month_remainder_days) == rint(month_remainder_days)) + month_remainder_days = rint(month_remainder_days); result-day += (int32) month_remainder_days; Don't we want to be checking for rounding at the usec level rather than 0.01 of a day? I think this should be 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); Another question I have concerns the month_remainder_days != (int32) month_remainder_days comparison. If I understand it correctly, if the TSROUND == rint portion is true, the first part is true. Or is this just a quick, fast check to see if it's necessary to do a more computationally intensive check? TSROUND isn't defined for HAVE_INT64_TIMESTAMP. My first attempt at performing a corresponding comparison doesn't work: + if (month_remainder_days != (int32) month_remainder_days + #ifdef HAVE_INT64_TIMESTAMP + rint(month_remainder_days * USECS_PER_DAY) == + (month_remainder_days * USECS_PER_DAY)) + #else + TSROUND(month_remainder_days * SECS_PER_DAY) == + rint(month_remainder_days * SECS_PER_DAY)) + #endif +
Re: [PATCHES] [HACKERS] Interval aggregate regression failure
Michael Glaesemann wrote:
On Aug 30, 2006, at 7:12 , Bruce Momjian wrote:
Here are the results using my newest patch:
test= select interval '41 mon 12 days 360:00' / 10 as quotient_a
, interval '41 mon -12 days -360:00' / 10 as quotient_b
, interval '-41 mon 12 days 360:00' / 10 as quotient_c
, interval '-41 mon -12 days -360:00' / 10 as quotient_d;
quotient_a | quotient_b|
quotient_c |quotient_d
+-
+---+---
4 mons 4 days 40:48:00 | 4 mons 2 days -40:48:00 | -4 mons -2
days +40:48:00 | -4 mons -4 days -40:48:00
(1 row)
test= 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 12 days 122:24:00 | 1 year 6 days -122:24:00 | -1 years -6
days +122:24:00 | -1 years -12 days -122:24:00
(1 row)
I see no 23:60 entries.
Using Bruce's newest patch, I still get the 23:60 entries on my
machine (no integer-datetimes)
Strange, I do not see that here. Is there something wrong with our
hour/minute display? Someone posted a patch a few days ago for that.
Here is a test program. What does it show for you?
#include stdio.h
int
main(int argc, char *argv[])
{
double x;
x = 41;
x = x / 10.0;
printf(%f\n, x);
x = x - (int)x;
x = x * 30;
printf(%15.15f\n, x);
x = 0.1 * 30;
printf(%15.15f\n, x);
return 0;
}
The output for me is:
4.100
2.989
3.000
select version();
version
-
PostgreSQL 8.2devel on powerpc-apple-darwin8.7.0, compiled by GCC
powerpc-apple-darwin8-gcc-4.0.1 (GCC) 4.0.1 (Apple Computer, Inc.
build 5341)
(1 row)
Powerpc. Hmmm. I am on Intel.
select interval '41 mon 12 days 360:00' / 10 as quotient_a
, interval '41 mon -12 days -360:00' / 10 as quotient_b
, interval '-41 mon 12 days 360:00' / 10 as quotient_c
, interval '-41 mon -12 days -360:00' / 10 as quotient_d;
quotient_a | quotient_b|
quotient_c |quotient_d
+-
+---+---
4 mons 4 days 40:48:00 | 4 mons 2 days -40:48:00 | -4 mons -2 days
+40:48:00 | -4 mons -4 days -40:48:00
(1 row)
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 12 days 122:24:00 | 1 year 6 days -122:23:60.00 | -1 years -6
days +122:24:00 | -1 years -12 days -122:23:60.00
(1 row)
Yea, I see that -122:23:60.00.
The code assume if it is within 0.01 of a whole number, it
should be
rounded to a whole number. Patch attached with comments added.
/* 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 0.01 of an integer, we round them to integers.
+*/
+ if (month_remainder_days != (int32)month_remainder_days
+ TSROUND(month_remainder_days) == rint(month_remainder_days))
+ month_remainder_days = rint(month_remainder_days);
result-day += (int32) month_remainder_days;
Don't we want to be checking for rounding at the usec level rather
than 0.01 of a day? I think this should be
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 =
Re: [PATCHES] [HACKERS] Interval aggregate regression failure (expected seems
On Aug 30, 2006, at 12:50 , Bruce Momjian wrote:
Here is a test program. What does it show for you?
The output for me is:
4.100
2.989
3.000
Here's what I get. Just to make sure I'm doing this right, I'm
including how I compiled it.
$ cat div_test.c
#include stdio.h
int
main(int argc, char *argv[])
{
double x;
x = 41;
x = x / 10.0;
printf(%f\n, x);
x = x - (int)x;
x = x * 30;
printf(%15.15f\n, x);
x = 0.1 * 30;
printf(%15.15f\n, x);
return 0;
}
$ gcc div_test.c -o div_test
$ ./div_test
4.10
2.989
3.000
$
Yea, just an optimization, but I was worried that the computations
might
throw problems for certain numbers, so I figured I would only
trigger it
when necessary.
Thanks for the explanation. Helps me know I might actually be
learning this.
Patch attached. It also fixes a regression test output too.
Thanks for the patch. I'll look at it more closely tonight.
Michael Glaesemann
grzm seespotcode net
---(end of broadcast)---
TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] [HACKERS] Interval aggregate regression failure (expected seems
On Aug 26, 2006, at 11:40 , Bruce Momjian wrote:
I used your ideas to make a patch to fix your example:
test= select '41 months'::interval / 10;
?column?
---
4 mons 3 days
(1 row)
and
test= select '41 months'::interval * 0.3;
?column?
---
1 year 9 days
(1 row)
The trick was not to play with the division, but to check if the
number
of seconds cascaded into full days and/or months.
While this does provide a fix for the example, I don't believe it's a
complete solution. For example, with your patch, you also get the
following results:
select '41 mon 360:00'::interval / 10 as pos
, '-41 mon -360:00'::interval / 10 as neg;
pos | neg
+--
4 mons 2 days 60:00:00 | -4 mons -2 days -59:59:60.00
(1 row)
If I've done the math right, this should be:
4 mons 3 days 36:00:00 | -4 mons -3 days -36:00:00
select '41 mon -360:00'::interval / 10 as pos
, '-41 mon 360:00'::interval / 10 as neg;
pos |neg
-+---
4 mons 2 days -12:00:00 | -4 mons -2 days +12:00:00
(1 row)
Should be:
4 mons 3 days -36:00:00 | -4 mons -3 days +36:00:00
What we want to do is check just the month contribution to the day
component to see if it is greater than 24 hours. Perhaps the simplest
way to accomplish this is something like (psuedo code):
if (abs(tsround(month_remainder * SECS_PER_DAY)) == SECS_PER_DAY)
{
if (month_remainder 0)
{
result-month++;
}
else
{
result-month--;
}
}
I'm going to try something along these lines this evening.
FWIW, I've included the patch of for what I'm working on. It's pretty
heavily commented right now with expected results as I think through
what the code's doing. (It also includes the DecodeInterval patch I
sent to -patches earlier today.) I'm still getting overflow warnings
in the lines including USECS_PER_DAY and USECS_PER_MONTH, and my
inexperience with C and gdb is getting the best of me right now
(though I'm still plugging away: ).
Michael Glaesemann
grzm seespotcode net
8---
? CONFIGURE_ARGS
? datetime.patch
? timestamp.patch
? src/backend/.DS_Store
? src/include/.DS_Store
? src/test/.DS_Store
Index: src/backend/utils/adt/datetime.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/utils/adt/datetime.c,v
retrieving revision 1.169
diff -c -r1.169 datetime.c
*** src/backend/utils/adt/datetime.c25 Jul 2006 03:51:21 - 1.169
--- src/backend/utils/adt/datetime.c28 Aug 2006 07:08:46 -
***
*** 2920,2935
tm-tm_mday += val * 7;
if (fval != 0)
{
! int
sec;
!
! fval *= 7 *
SECS_PER_DAY;
! sec = fval;
! tm-tm_sec += sec;
#ifdef HAVE_INT64_TIMESTAMP
! *fsec += (fval - sec) *
100;
#else
! *fsec += fval - sec;
#endif
}
tmask = (fmask DTK_M(DAY)) ?
0 : DTK_M(DAY);
break;
--- 2920,2942
tm-tm_mday += val * 7;
if (fval != 0)
{
! int extra_days;
! fval *= 7;
! extra_days = (int32)
fval;
! tm-tm_mday +=
extra_days;
! fval -= extra_days;
! if (fval != 0)
! {
! int
sec;
! fval *=
SECS_PER_DAY;
! sec = fval;
! tm-tm_sec +=
sec;
#ifdef HAVE_INT64_TIMESTAMP
! *fsec += (fval
- sec) * 100;
#else
!
Re: [PATCHES] [HACKERS] Interval aggregate regression failure (expected
I used your ideas to make a patch to fix your example:
test= select '41 months'::interval / 10;
?column?
---
4 mons 3 days
(1 row)
and
test= select '41 months'::interval * 0.3;
?column?
---
1 year 9 days
(1 row)
The trick was not to play with the division, but to check if the number
of seconds cascaded into full days and/or months.
---
Tom Lane wrote:
Michael Glaesemann [EMAIL PROTECTED] writes:
... I think this just confirms that there is some kind of rounding (or
lack of) in interval_div. Kind of frustrating that it's not visible
in the result.
I think the fundamental problem is that the float8 results of division
are inaccurate, and yet we're assuming that we can (for instance) coerce
them to integer and get exactly the right answer. For instance, in the
'41 months'/10 example, I get month_remainder_days being computed as
(gdb) p month_remainder
$19 = 0.099645
(gdb) s
2575result-day += (int32) month_remainder_days;
(gdb) p month_remainder_days
$20 = 2.9893
The only way we can really fix this is to be willing to round off
the numbers, and I think the only principled way to do that is to
settle on a specific target accuracy, probably 1 microsecond.
Then the thing to do would be to scale up all the intermediate
float results to microseconds and apply rint(). Something like
(untested)
month_remainder = rint(span-month * USECS_PER_MONTH / factor);
day_remainder = rint(span-day * USECS_PER_DAY / factor);
result-month = (int32) (month_remainder / USECS_PER_MONTH);
result-day = (int32) (day_remainder / USECS_PER_DAY);
month_remainder -= result-month * USECS_PER_MONTH;
day_remainder -= result-day * USECS_PER_DAY;
/*
* Handle any fractional parts the same way as in interval_mul.
*/
/* fractional months full days into days */
month_remainder_days = month_remainder * DAYS_PER_MONTH;
extra_days = (int32) (month_remainder_days / USECS_PER_DAY);
result-day += extra_days;
/* fractional months partial days into time */
day_remainder += month_remainder_days - extra_days * USECS_PER_DAY;
#ifdef HAVE_INT64_TIMESTAMP
result-time = rint(span-time / factor + day_remainder);
#else
result-time = rint(span-time * 1.0e6 / factor + day_remainder) /
1.0e6;
#endif
This might need a few more rint() calls --- I'm assuming that float ops
with exact integral inputs will be OK, which is an assumption used
pretty widely in the datetime code, but ...
Per the comment, if we do this here we probably want to make
interval_mul work similarly.
regards, tom lane
--
Bruce Momjian [EMAIL PROTECTED]
EnterpriseDBhttp://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Index: src/backend/utils/adt/timestamp.c
===
RCS file: /cvsroot/pgsql/src/backend/utils/adt/timestamp.c,v
retrieving revision 1.165
diff -c -c -r1.165 timestamp.c
*** src/backend/utils/adt/timestamp.c 13 Jul 2006 16:49:16 - 1.165
--- src/backend/utils/adt/timestamp.c 26 Aug 2006 02:36:28 -
***
*** 2526,2531
--- 2526,2546
result-time = rint(span-time * factor + day_remainder * USECS_PER_DAY);
#else
result-time = span-time * factor + day_remainder * SECS_PER_DAY;
+ /*
+ * The imprecision of float8 causes unusual rounding of even integer
+ * division, so round up if we have full units. Check seconds only
+ * as far as microseconds.
+ */
+ if (TSROUND(result-time) == SECS_PER_DAY)
+ {
+ result-day++;
+ result-time = 0;
+ }
+ if (result-day == DAYS_PER_MONTH)
+ {
+ result-month++;
+ result-day = 0;
+ }
#endif
PG_RETURN_INTERVAL_P(result);
***
*** 2579,2584
--- 2594,2614
result-time = rint(span-time / factor + day_remainder * USECS_PER_DAY);
#else
result-time = span-time / factor + day_remainder * SECS_PER_DAY;
+ /*
+ * The imprecision of float8 causes unusual rounding of even integer
+ * division, so round up if we have full units. Check seconds only
+ * as far as microseconds.
+ */
+ if (TSROUND(result-time) == SECS_PER_DAY)
+ {
+ result-day++;
+ result-time = 0;
+ }
+ if (result-day == DAYS_PER_MONTH)
+ {
+ result-month++;
+ result-day = 0;
+ }
#endif
PG_RETURN_INTERVAL_P(result);
---(end of broadcast)---
TIP 5: don't forget to increase your free space map settings
