Thread: Re: [PATCHES] Interval aggregate regression failure
Bruce Momjian <bruce@momjian.us> 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.
regards, tom lane
Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> 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 bruce@momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian <bruce@momjian.us> 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
Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> 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 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 5:05 , Bruce Momjian wrote:
> Tom Lane wrote:
>> Bruce Momjian <bruce@momjian.us> 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 10000 * '1000000 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 10000 * '1000000 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
------------------------+---------------------------
+---------------------------+---------------------------
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:24:00
(1 row)
select 10000 * '1000000 hours'::interval as "ten billion";
ten billion
------------------
-00:00:00.000001
(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
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 <bruce@momjian.us> 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 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: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
Please ignore the patch I just sent. Much too quick with the send button. Michael Glaesemann grzm seespotcode net
Michael Glaesemann <grzm@seespotcode.net> 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