Thread

Interval aggregate regression failure (expected seems wrong)

Michael Paesold <mpaesold@gmx.at> — 2005-11-07T08:24:08Z

Using both PostgreSQL 8.1.0 and CVS current of Nov 7, 9:00 am CET I get 
a regression failure in the interval tests. I am no export for the 
interval type, but the expected "9 days 28 hours" seem wrong, don't 
they? The actual value seems to be the same.

Is it possible that this is broken on the platform where the expected 
results were generated?

*** ./expected/interval.out Tue Oct 25 19:13:07 2005
--- ./results/interval.out  Mon Nov  7 09:11:27 2005
***************
*** 218,224 ****
   select avg(f1) from interval_tbl;
                          avg
   -------------------------------------------------
!  @ 4 years 1 mon 9 days 28 hours 18 mins 23 secs
   (1 row)

   -- test long interval input
--- 218,224 ----
   select avg(f1) from interval_tbl;
                          avg
   -------------------------------------------------
!  @ 4 years 1 mon 10 days 4 hours 18 mins 23 secs
   (1 row)

   -- test long interval input

The last commit to interval.out seems to be this one, and it changed 
exactly this line.
revision 1.14
date: 2005/10/25 17:13:07;  author: tgl;  state: Exp;  lines: +1 -1

Well, this is CVS tip, so there is a chance this is fixed in 
REL_8_1_STABLE which has a 1.14.0.2. At least the release tarball should 
be rebuilt, no?
Sorry, if this is just noise. Just wanted to be sure you know about it.

Best Regards,
Michael

Re: Interval aggregate regression failure (expected seems wrong)

Michael Glaesemann <grzm@myrealbox.com> — 2005-11-07T08:48:07Z

On Nov 7, 2005, at 17:24 , Michael Paesold wrote:

> Using both PostgreSQL 8.1.0 and CVS current of Nov 7, 9:00 am CET I  
> get a regression failure in the interval tests. I am no export for  
> the interval type, but the expected "9 days 28 hours" seem wrong,  
> don't they? The actual value seems to be the same.
>
> Is it possible that this is broken on the platform where the  
> expected results were generated?

What platform are you testing on? With or without integer-datetimes?

I just ran make check on for PostgreSQL 8.1.0 on Mac OS X 10.4.3

test=# select version();

version
------------------------------------------------------------------------ 
----------------------------------------------------------------------
PostgreSQL 8.1.0 on powerpc-apple-darwin8.3.0, compiled by GCC  
powerpc-apple-darwin8-gcc-4.0.0 (GCC) 4.0.0 (Apple Computer, Inc.  
build 5026)

I didn't have any regression failures. I'd also expect we'd see a lot  
more failures on the build farm if it were the case that it was  
broken just on the platform that the expected results were generated  
on. From a quick look at the current build farm failures on HEAD and  
REL8_1_STABLE, it doesn't look like any of the failures are failing  
here.

Michael Glaesemann
grzm myrealbox com

Re: Interval aggregate regression failure (expected seems

Michael Paesold <mpaesold@gmx.at> — 2005-11-07T09:28:56Z

Michael Glaesemann wrote:
> 
> On Nov 7, 2005, at 17:24 , Michael Paesold wrote:
> 
>> Using both PostgreSQL 8.1.0 and CVS current of Nov 7, 9:00 am CET I  
>> get a regression failure in the interval tests. I am no export for  
>> the interval type, but the expected "9 days 28 hours" seem wrong,  
>> don't they? The actual value seems to be the same.
>>
>> Is it possible that this is broken on the platform where the  expected 
>> results were generated?
> 
> What platform are you testing on? With or without integer-datetimes?

Ok, forgot. This is *without* integer-datetimes, RHEL 3 (Linux 2.4.21, 
glibc 2.3.2, gcc 3.2.3 20030502) on i686 (Xeon without x86-64).

> I just ran make check on for PostgreSQL 8.1.0 on Mac OS X 10.4.3
[snip]
> I didn't have any regression failures. I'd also expect we'd see a lot  
> more failures on the build farm if it were the case that it was  broken 
> just on the platform that the expected results were generated  on. From 
> a quick look at the current build farm failures on HEAD and  
> REL8_1_STABLE, it doesn't look like any of the failures are failing  here.

I just started to wonder about buildfarm, too, but found that most build 
farm members have --enable-integer-datetimes. Could that be an 
explanation? Is it possible that the code is wrong with 
--enable-integer-datetimes?

So what do you have in results/interval.out?
@ 4 years 1 mon 9 days 28 hours 18 mins 23 secs seems wrong to me, no?

Tom wrote for that commit:
revision 1.14
date: 2005/10/25 17:13:07;  author: tgl;  state: Exp;  lines: +1 -1
Remove justify_hours call from interval_mul and interval_div, and make
some small stylistic improvements in these functions.  Also fix several
places where TMODULO() was being used with wrong-sized quotient argument,
creating a risk of overflow --- interval2tm was actually capable of going
into an infinite loop because of this.

Perhaps it is an intended behavior? If so, it still fails without 
integer-datetimes.

Best Regards,
Michael Paesold

Re: Interval aggregate regression failure (expected seems

Michael Paesold <mpaesold@gmx.at> — 2005-11-07T09:42:08Z


Michael Paesold wrote:
>> On Nov 7, 2005, at 17:24 , Michael Paesold wrote:
>>
>>> Using both PostgreSQL 8.1.0 and CVS current of Nov 7, 9:00 am CET I  
>>> get a regression failure in the interval tests. I am no export for  
>>> the interval type, but the expected "9 days 28 hours" seem wrong,  
>>> don't they? The actual value seems to be the same.
>>>
>>> Is it possible that this is broken on the platform where the  
>>> expected results were generated?

> Perhaps it is an intended behavior? If so, it still fails without 
> integer-datetimes.

Well, no, it also fails with integer-datetimes for me in the same way.
pg_config output below. And yes, I did "cvs up; make distclean; 
./configure... ; make ; make install ; make check".

Could this be DST-related? I thought plain interval was not affected by 
DST changes.

BINDIR = /usr/local/postgresql-8cvs/bin
DOCDIR = /usr/local/postgresql-8cvs/doc
INCLUDEDIR = /usr/local/postgresql-8cvs/include
PKGINCLUDEDIR = /usr/local/postgresql-8cvs/include
INCLUDEDIR-SERVER = /usr/local/postgresql-8cvs/include/server
LIBDIR = /usr/local/postgresql-8cvs/lib
PKGLIBDIR = /usr/local/postgresql-8cvs/lib
LOCALEDIR =
MANDIR = /usr/local/postgresql-8cvs/man
SHAREDIR = /usr/local/postgresql-8cvs/share
SYSCONFDIR = /usr/local/postgresql-8cvs/etc
PGXS = /usr/local/postgresql-8cvs/lib/pgxs/src/makefiles/pgxs.mk
CONFIGURE = '--prefix=/usr/local/postgresql-8cvs' '--with-pgport=54321' 
'--with-perl' 'CFLAGS=-O2 -mcpu=pentium4 -march=pentium4' 
'--enable-casserts' '--enable-debug' '--enable-integer-datetimes'
CC = gcc
CPPFLAGS = -D_GNU_SOURCE
CFLAGS = -O2 -mcpu=pentium4 -march=pentium4 -Wall -Wmissing-prototypes 
-Wpointer-arith -Winline -Wdeclaration-after-statement 
-fno-strict-aliasing -g
CFLAGS_SL = -fpic
LDFLAGS = -Wl,-rpath,/usr/local/postgresql-8cvs/lib
LDFLAGS_SL =
LIBS = -lpgport -lz -lreadline -lncurses -lcrypt -lresolv -lnsl -ldl -lm 
-lbsd
VERSION = PostgreSQL 8.2devel

Best Regards,
Michael Paesold

Re: Interval aggregate regression failure (expected seems wrong)

Michael Glaesemann <grzm@myrealbox.com> — 2005-11-07T09:59:28Z

On Nov 7, 2005, at 18:28 , Michael Paesold wrote:

> Ok, forgot. This is *without* integer-datetimes, RHEL 3 (Linux  
> 2.4.21, glibc 2.3.2, gcc 3.2.3 20030502) on i686 (Xeon without  
> x86-64).
>
>> I just ran make check on for PostgreSQL 8.1.0 on Mac OS X 10.4.3

Heh. I forgot, too ;) My test was also without integer-datetimes.

> [snip]
>> I didn't have any regression failures. I'd also expect we'd see a  
>> lot  more failures on the build farm if it were the case that it  
>> was  broken just on the platform that the expected results were  
>> generated  on. From a quick look at the current build farm  
>> failures on HEAD and  REL8_1_STABLE, it doesn't look like any of  
>> the failures are failing  here.
>
> I just started to wonder about buildfarm, too, but found that most  
> build farm members have --enable-integer-datetimes. Could that be  
> an explanation? Is it possible that the code is wrong with --enable- 
> integer-datetimes?
>
> So what do you have in results/interval.out?
> @ 4 years 1 mon 9 days 28 hours 18 mins 23 secs seems wrong to me, no?
>

select avg(f1) from interval_tbl;
                        avg
-------------------------------------------------
@ 4 years 1 mon 9 days 28 hours 18 mins 23 secs
(1 row)

The point of the change to the interval datatype in 8.1 is to keep  
track of months, days, and seconds (which in turn are represented as  
hours, minutes and seconds). Previous releases tracked only months  
and seconds. This has advantages for using intervals with dates and  
timestamps that involve daylight saving time changes. Admittedly, it  
looks odd at first, but it falls out of the change in behavior of the  
interval datatype. There are two new functions, justify_days and  
justify_hours, that you can use to put intervals into more  
traditional forms.

http://developer.postgresql.org/docs/postgres/functions-datetime.html

Doesn't explain why you're getting a regression failure though.

Michael Glaesemann
grzm myrealbox com

Re: Interval aggregate regression failure (expected seems

Michael Paesold <mpaesold@gmx.at> — 2005-11-07T10:21:50Z

Michael Glaesemann wrote:
>> So what do you have in results/interval.out?
>> @ 4 years 1 mon 9 days 28 hours 18 mins 23 secs seems wrong to me, no?
>>
> 
> select avg(f1) from interval_tbl;
>                        avg
> -------------------------------------------------
> @ 4 years 1 mon 9 days 28 hours 18 mins 23 secs
> (1 row)
> 
> The point of the change to the interval datatype in 8.1 is to keep  
> track of months, days, and seconds (which in turn are represented as  
> hours, minutes and seconds). Previous releases tracked only months  and 
> seconds. This has advantages for using intervals with dates and  
> timestamps that involve daylight saving time changes. Admittedly, it  
> looks odd at first, but it falls out of the change in behavior of the  
> interval datatype. There are two new functions, justify_days and  
> justify_hours, that you can use to put intervals into more  traditional 
> forms.
> 
> http://developer.postgresql.org/docs/postgres/functions-datetime.html

Thank you very much for the insight.

> Doesn't explain why you're getting a regression failure though.

Well, I have something now. It seems to be a compiler/optimization issue.

I wrote:
 > CFLAGS = -O2 -mcpu=pentium4 -march=pentium4 -Wall -Wmissing-prototypes
 > -Wpointer-arith -Winline -Wdeclaration-after-statement
 > -fno-strict-aliasing -g

I had set CFLAGS to -O2 -mcpu=pentium4 -march=pentium4. I have been 
using these settings for testing PostgreSQL tip for some time now and 
never had any problems.

Removing the cpu and architecture optimization part changes the behavior 
of the interval aggrate, so the results/interval.out now also looks like 
the expected output.
select avg(f1) from interval_tbl;
                        avg
-------------------------------------------------
  @ 4 years 1 mon 9 days 28 hours 18 mins 23 secs
(1 row)

Switching -mcpu=pentium4 -march=pentium4 back on, results in wrong 
output. This is 100% reproducable. Can somebody with more knowledge 
explain why the compiler should stumble over just this? Pure luck?

I have tested these combination of CFLAGS:
-O2                                 OK
-O2 -mcpu=i686 -march=i686          OK (good, RPMS are built with these)
-O2 -mcpu=pentium4 -march=i686      OK
-O2 -mcpu=pentium4 -march=pentium4  fails

I am definatly not going to use -march=pentium4 in any production 
system. Should I open a bug report with RedHat (gcc vendor)?

Best Regards,
Michael Paesold

Re: Interval aggregate regression failure (expected seems

Tom Lane <tgl@sss.pgh.pa.us> — 2005-11-07T14:31:46Z

Michael Paesold <mpaesold@gmx.at> writes:
> I have tested these combination of CFLAGS:
> -O2                                 OK
> -O2 -mcpu=i686 -march=i686          OK (good, RPMS are built with these)
> -O2 -mcpu=pentium4 -march=i686      OK
> -O2 -mcpu=pentium4 -march=pentium4  fails

> I am definatly not going to use -march=pentium4 in any production 
> system. Should I open a bug report with RedHat (gcc vendor)?

Yeah, but they'll probably want a smaller test case than "Postgres fails
its regression tests" :-(

My guess is that the problem is misoptimization of the interval_div()
function (look in utils/adt/timestamp.c), and that you could probably
construct a nice small test case for them out of that function.

			regards, tom lane

Re: Interval aggregate regression failure (expected seems

Tom Lane <tgl@sss.pgh.pa.us> — 2005-11-07T17:07:27Z

I wrote:
> Michael Paesold <mpaesold@gmx.at> writes:
>> I am definatly not going to use -march=pentium4 in any production 
>> system. Should I open a bug report with RedHat (gcc vendor)?

> Yeah, but they'll probably want a smaller test case than "Postgres fails
> its regression tests" :-(

I have just confirmed that the problem still exists in FC4's current
compiler (gcc 4.0.1, gcc-4.0.1-4.fc4), which probably will boost up the
priority of the complaint quite a long way in Red Hat's eyes.

I've also confirmed that the problem is in interval_div; you can
reproduce the failure with

	select '41 years 1 mon 11 days'::interval / 10;

which should give '4 years 1 mon 9 days 26:24:00', but when
timestamp.o is compiled with "-mcpu=pentium4 -march=pentium4",
you get '4 years 1 mon 10 days 02:24:00'.  --enable-integer-datetimes
is not relevant because the interesting part is all double/integer
arithmetic.

Looking at this, though, I wonder if the pentium4 answer isn't "more
correct".  If I'm doing the math by hand correctly, what we end up
with is having to cascade 3/10ths of a month down into the days field,
and since the conversion factor is supposed to be 30 days/month, that
should be exactly 9 days.  Plus the one full day from the 11/10 days
gives 10 days.  I think what is happening on all the non-Pentium
platforms is that (3.0/10.0)*30.0 is producing something just a shade
less than 9.0, whereas the Pentium gives 9.0 or a shade over, possibly
due to rearrangement of the calculation.  I think we can still file this
as a compiler bug, because I'm pretty sure the C spec does not allow
rearrangement of floating-point calculations ... but we might want to
think about tweaking the code's roundoff behavior just a bit.

An example that's a little easier to look at is

	select '41 years 1 mon'::interval / 10;

I get '4 years 1 mon 9 days' with the pentium4 optimization, and
'4 years 1 mon 8 days 24:00:00' without, and the former seems more
correct...

			regards, tom lane

Re: Interval aggregate regression failure (expected seems

Greg Stark <gsstark@mit.edu> — 2005-11-07T19:22:37Z

Tom Lane <tgl@sss.pgh.pa.us> writes:

> I think we can still file this as a compiler bug, because I'm pretty sure
> the C spec does not allow rearrangement of floating-point calculations ...

It may have more to do with whether the floating point value can stay in a
floating point register long enough to complete the calculation. 

IIRC, floating point registers are actually longer than a double so if the
entire calculation is done in registers and then the result rounded off to
store in memory it may get the right answer. Whereas if it loses the extra
bits on the intermediate values (the infinite repeating fractions) that might
be where you get the imprecise results.

It makes some sense that -mcpu and -march give the compiler enough information
to keep the intermediate results in registers more effectively.

-- 
greg

Re: Interval aggregate regression failure (expected seems

Greg Stark <gsstark@mit.edu> — 2005-11-07T19:25:35Z

c.f.:


`-ffloat-store'
     Do not store floating point variables in registers, and inhibit
     other options that might change whether a floating point value is
     taken from a register or memory.

     This option prevents undesirable excess precision on machines such
     as the 68000 where the floating registers (of the 68881) keep more
     precision than a `double' is supposed to have.  Similarly for the
     x86 architecture.  For most programs, the excess precision does
     only good, but a few programs rely on the precise definition of
     IEEE floating point.  Use `-ffloat-store' for such programs, after
     modifying them to store all pertinent intermediate computations
     into variables.

-- 
greg

Re: Interval aggregate regression failure (expected seems

Gregory Maxwell <gmaxwell@gmail.com> — 2005-11-07T20:07:15Z

On 07 Nov 2005 14:22:37 -0500, Greg Stark <gsstark@mit.edu> wrote:
> IIRC, floating point registers are actually longer than a double so if the
> entire calculation is done in registers and then the result rounded off to
> store in memory it may get the right answer. Whereas if it loses the extra
> bits on the intermediate values (the infinite repeating fractions) that might
> be where you get the imprecise results.

Hm. I thought -march=pentium4 -mcpu=pentium4 implies -mfpmath=sse. 
SSE is a much better choice on P4 for performance reasons, and never
has excess precision. I'm guessing from the above that I'm incorrect,
in which case we should always be compiled with -mfpmath=sse -msse2
when we are complied -march=pentium4, this should remove problems
caused by excess precision. The same behavior can be had on non sse
platforms with -ffloat-store.

Re: Interval aggregate regression failure (expected seems

Michael Paesold <mpaesold@gmx.at> — 2005-11-08T21:07:35Z

Gregory Maxwell wrote:
> On 07 Nov 2005 14:22:37 -0500, Greg Stark <gsstark@mit.edu> wrote:
> 
>>IIRC, floating point registers are actually longer than a double so if the
>>entire calculation is done in registers and then the result rounded off to
>>store in memory it may get the right answer. Whereas if it loses the extra
>>bits on the intermediate values (the infinite repeating fractions) that might
>>be where you get the imprecise results.
> 
> 
> Hm. I thought -march=pentium4 -mcpu=pentium4 implies -mfpmath=sse. 
> SSE is a much better choice on P4 for performance reasons, and never
> has excess precision. I'm guessing from the above that I'm incorrect,
> in which case we should always be compiled with -mfpmath=sse -msse2
> when we are complied -march=pentium4, this should remove problems
> caused by excess precision. The same behavior can be had on non sse
> platforms with -ffloat-store.

Just for the record (and those interested): using 'CFLAGS=-O2 
-mcpu=pentium4 -march=pentium4 -mfpmath=sse -msse2' actually passes the 
regression tests.

Best Regards,
Michael Paesold

Re: Interval aggregate regression failure (expected seems

Michael Paesold <mpaesold@gmx.at> — 2005-11-08T21:13:17Z

Tom Lane wrote:
> I wrote:
> 
>>Michael Paesold <mpaesold@gmx.at> writes:
>>
>>>I am definatly not going to use -march=pentium4 in any production 
>>>system. Should I open a bug report with RedHat (gcc vendor)?
> 
> 
>>Yeah, but they'll probably want a smaller test case than "Postgres fails
>>its regression tests" :-(
> 
> 
> I have just confirmed that the problem still exists in FC4's current
> compiler (gcc 4.0.1, gcc-4.0.1-4.fc4), which probably will boost up the
> priority of the complaint quite a long way in Red Hat's eyes.
> 
> I've also confirmed that the problem is in interval_div; you can
> reproduce the failure with
> 
> 	select '41 years 1 mon 11 days'::interval / 10;
[snip]

Would you mind reporting this to RedHat Bugzilla? I believe a bug report 
from you would have more weight then mine, because you actually 
understand what's going on here. :-)

Otherwise I am going to do do my best...

Best Regards,
Michael Paesold

Re: Interval aggregate regression failure (expected seems

Tom Lane <tgl@sss.pgh.pa.us> — 2005-11-08T22:48:16Z

Michael Paesold <mpaesold@gmx.at> writes:
> Would you mind reporting this to RedHat Bugzilla? I believe a bug report 
> from you would have more weight then mine, because you actually 
> understand what's going on here. :-)

Actually, given the thought that this may be an artifact of keeping an
intermediate value in a wider-than-normal register rather than genuinely
rearranging the computation, I'm not certain it is a compiler bug.
We'd have to study it a lot more closely before filing it as one, anyway.

If you accept the idea that the pentium4 answer is the right one,
then what we really need to do is focus on a better rounding rule than
"strict truncation".  I was toying with the notion of adding the
equivalent of half a microsecond to the fractional-day value before
truncating it to integer.  But I'm not certain that that wouldn't have
some bad effects in other cases.

			regards, tom lane

Re: Interval aggregate regression failure (expected seems

Bruce Momjian <pgman@candle.pha.pa.us> — 2005-11-15T03:07:49Z

Tom Lane wrote:
> Michael Paesold <mpaesold@gmx.at> writes:
> > Would you mind reporting this to RedHat Bugzilla? I believe a bug report 
> > from you would have more weight then mine, because you actually 
> > understand what's going on here. :-)
> 
> Actually, given the thought that this may be an artifact of keeping an
> intermediate value in a wider-than-normal register rather than genuinely
> rearranging the computation, I'm not certain it is a compiler bug.
> We'd have to study it a lot more closely before filing it as one, anyway.
> 
> If you accept the idea that the pentium4 answer is the right one,
> then what we really need to do is focus on a better rounding rule than
> "strict truncation".  I was toying with the notion of adding the
> equivalent of half a microsecond to the fractional-day value before
> truncating it to integer.  But I'm not certain that that wouldn't have
> some bad effects in other cases.

Looking at the code, do we need additional rint() calls in there, or
rint(x + 0.5)?  Frankly, I am confused why interval_div() has caused
such problems for us?  Are we going at this the right way?

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Re: Interval aggregate regression failure (expected seems

Bruce Momjian <pgman@candle.pha.pa.us> — 2006-06-14T22:52:56Z

I assume no more progress has been made on this?

---------------------------------------------------------------------------

Tom Lane wrote:
> I wrote:
> > Michael Paesold <mpaesold@gmx.at> writes:
> >> I am definatly not going to use -march=pentium4 in any production 
> >> system. Should I open a bug report with RedHat (gcc vendor)?
> 
> > Yeah, but they'll probably want a smaller test case than "Postgres fails
> > its regression tests" :-(
> 
> I have just confirmed that the problem still exists in FC4's current
> compiler (gcc 4.0.1, gcc-4.0.1-4.fc4), which probably will boost up the
> priority of the complaint quite a long way in Red Hat's eyes.
> 
> I've also confirmed that the problem is in interval_div; you can
> reproduce the failure with
> 
> 	select '41 years 1 mon 11 days'::interval / 10;
> 
> which should give '4 years 1 mon 9 days 26:24:00', but when
> timestamp.o is compiled with "-mcpu=pentium4 -march=pentium4",
> you get '4 years 1 mon 10 days 02:24:00'.  --enable-integer-datetimes
> is not relevant because the interesting part is all double/integer
> arithmetic.
> 
> Looking at this, though, I wonder if the pentium4 answer isn't "more
> correct".  If I'm doing the math by hand correctly, what we end up
> with is having to cascade 3/10ths of a month down into the days field,
> and since the conversion factor is supposed to be 30 days/month, that
> should be exactly 9 days.  Plus the one full day from the 11/10 days
> gives 10 days.  I think what is happening on all the non-Pentium
> platforms is that (3.0/10.0)*30.0 is producing something just a shade
> less than 9.0, whereas the Pentium gives 9.0 or a shade over, possibly
> due to rearrangement of the calculation.  I think we can still file this
> as a compiler bug, because I'm pretty sure the C spec does not allow
> rearrangement of floating-point calculations ... but we might want to
> think about tweaking the code's roundoff behavior just a bit.
> 
> An example that's a little easier to look at is
> 
> 	select '41 years 1 mon'::interval / 10;
> 
> I get '4 years 1 mon 9 days' with the pentium4 optimization, and
> '4 years 1 mon 8 days 24:00:00' without, and the former seems more
> correct...
> 
> 			regards, tom lane
> 
> ---------------------------(end of broadcast)---------------------------
> TIP 5: don't forget to increase your free space map settings
> 

-- 
  Bruce Momjian   http://candle.pha.pa.us
  EnterpriseDB    http://www.enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +

Re: Interval aggregate regression failure (expected seems

Michael Glaesemann <grzm@myrealbox.com> — 2006-06-23T00:47:13Z

Tom Lane wrote:

> I've also confirmed that the problem is in interval_div; you can
> reproduce the failure with
>
> 	select '41 years 1 mon 11 days'::interval / 10;
>
> which should give '4 years 1 mon 9 days 26:24:00', but when
> timestamp.o is compiled with "-mcpu=pentium4 -march=pentium4",
> you get '4 years 1 mon 10 days 02:24:00'.  --enable-integer-datetimes
> is not relevant because the interesting part is all double/integer
> arithmetic.
>
> Looking at this, though, I wonder if the pentium4 answer isn't "more
> correct".  If I'm doing the math by hand correctly, what we end up
> with is having to cascade 3/10ths of a month down into the days field,
> and since the conversion factor is supposed to be 30 days/month, that
> should be exactly 9 days.  Plus the one full day from the 11/10 days
> gives 10 days.  I think what is happening on all the non-Pentium
> platforms is that (3.0/10.0)*30.0 is producing something just a shade
> less than 9.0, whereas the Pentium gives 9.0 or a shade over, possibly
> due to rearrangement of the calculation.  I think we can still file  
> this
> as a compiler bug, because I'm pretty sure the C spec does not allow
> rearrangement of floating-point calculations ... but we might want to
> think about tweaking the code's roundoff behavior just a bit.

I took a naive look at this, but have been unable to come up with a  
satisfactory solution. Here's an even simpler example that exhibits  
the behavior:

# select '41 mon'::interval / 10;
         ?column?
------------------------
4 mons 2 days 24:00:00

And one using a non-integer divisor:

# select '2 mon'::interval / 1.5;
        ?column?
-----------------------
1 mon 9 days 24:00:00

Here's the relevant code in interval_div (timestamp.c c2573).  
month_remainder holds the fractional part of the months field of the  
original interval (41) divided by the divisor (10) as a double.

	/* 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

My understanding is that as month_remainder is a float (as is  
month_remainder_days), month_remainder_days may be equal to 24 hours  
after rounding. As we're converting from months to days, and from  
days to time, rather than from months to time directly, we're  
assuming that we should only have time less than 24 hours remaining  
in the month_remainder_days when it's added to day_remainder. If  
month_remainder_days is equal to 24 hours, we should increment result- 
 >day rather than carrying that down into result->time.

With that in mind, I produced this hack:

#ifdef HAVE_INT64_TIMESTAMP
     month_remainder_seconds = month_remainder_day_fraction *  
USECS_PER_DAY;

     if ( USECS_PER_DAY == rint(month_remainder_seconds) )
         result->day++;
     else if ( USECS_PER_DAY == - rint(month_remainder_seconds) )
         result->day--;
     else day_remainder += month_remainder_day_fraction;

	result->time = rint(span->time / factor + day_remainder *  
USECS_PER_DAY);
#else
     month_remainder_seconds = month_remainder_day_fraction *  
SECS_PER_DAY;

     if ( SECS_PER_DAY == (int32) month_remainder_seconds )
         result->day++;
     else if ( SECS_PER_DAY == - (int32) month_remainder_seconds)
         result->day--;
     else day_remainder += month_remainder_day_fraction;

	result->time = span->time / factor + day_remainder * SECS_PER_DAY;
#endif


It works okay with --enable-integer-datetimes

postgres=# select '41 mon'::interval/10;
    ?column?
---------------
4 mons 3 days
(1 row)

postgres=# select '2 mon'::interval/1.5;
    ?column?
---------------
1 mon 10 days
(1 row)

It also changes the result of the aggregate test for intervals, but I  
think that's to be expected.

*** ./expected/interval.out	Tue Mar  7 07:49:17 2006
--- ./results/interval.out	Thu Jun 22 22:52:41 2006
***************
*** 218,224 ****
   select avg(f1) from interval_tbl;
                          avg
   -------------------------------------------------
!  @ 4 years 1 mon 9 days 28 hours 18 mins 23 secs
   (1 row)

   -- test long interval input
--- 218,224 ----
   select avg(f1) from interval_tbl;
                          avg
   -------------------------------------------------
!  @ 4 years 1 mon 10 days 4 hours 18 mins 23 secs
   (1 row)

   -- test long interval input


But doesn't work without --enable-integer-datetimes. It still gives  
the same answer as before. I don't have a firm grasp of floating  
point math so I'm fully aware this might be entirely the wrong way to  
go about this. It definitely feels kludgy (especially my sign- 
handling), but I submit this in the hope it moves the discussion  
forward a little. If someone wanted to point me in the right  
direction, I'll try to finish this up, updating the regression test  
and adding a few more to test this more thoroughly.

Thanks!

Michael Glaesemann
grzm myrealbox com

Re: Interval aggregate regression failure (expected seems

Michael Glaesemann <grzm@seespotcode.net> — 2006-06-23T00:53:31Z

On Jun 23, 2006, at 9:47 , Michael Glaesemann wrote:

> It also changes the result of the aggregate test for intervals, but  
> I think that's to be expected.

My goodness. Of course it changes the aggregate test results. That  
was what brought this up in the first place. (*kicks self for not  
reading the subject line when responding*)

Michael Glaesemann
grzm seespotcode net

Re: Interval aggregate regression failure (expected seems

Michael Glaesemann <grzm@seespotcode.net> — 2006-07-01T05:54:10Z

On Jun 23, 2006, at 9:47 , Michael Glaesemann wrote:

> # select '41 mon'::interval / 10;
>         ?column?
> ------------------------
> 4 mons 2 days 24:00:00

> My understanding is that as month_remainder is a float (as is  
> month_remainder_days), month_remainder_days may be equal to 24  
> hours after rounding. As we're converting from months to days, and  
> from days to time, rather than from months to time directly, we're  
> assuming that we should only have time less than 24 hours remaining  
> in the month_remainder_days when it's added to day_remainder.

This behavior is the same as applying justify_hours before adding the  
days and time components to the result. With this in mind, I rewrote  
interval_div to call interval_justify_hours. Good news is that -- 
enable-integer-datetimes works as expected. Bad news is that without  
--enable-integer-datetimes, still get the current behavior.

I also came across something I think is odd:

# select version();
                                                                     
version
------------------------------------------------------------------------ 
----------------------------------------------------------------------
PostgreSQL 8.1.4 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 justify_hours(a/10) as divided_and_justified
     , justify_hours(b) as justified
     , a/10 = b as are_equal
from (select '41 mon'::interval,'4 mons 2 days 24:00:00'::interval)  
as s(a,b);

without --enable-integer-datetimes:

divided_and_justified  |   justified   | are_equal
------------------------+---------------+-----------
4 mons 2 days 24:00:00 | 4 mons 3 days | t
(1 row)

with --enable-integer-datetimes:

divided_and_justified |   justified   | are_equal
-----------------------+---------------+-----------
4 mons 3 days         | 4 mons 3 days | t

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.

Anyway, there's another data point.

Michael Glaesemann
grzm seespotcode net

Re: Interval aggregate regression failure (expected seems

Tom Lane <tgl@sss.pgh.pa.us> — 2006-07-01T20:16:22Z

Michael Glaesemann <grzm@seespotcode.net> 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.099999999999999645
(gdb) s
2575            result->day += (int32) month_remainder_days;
(gdb) p month_remainder_days
$20 = 2.9999999999999893

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

Re: Interval aggregate regression failure (expected seems

Bruce Momjian <bruce@momjian.us> — 2006-08-11T04:48:08Z

Have we made any progress on this?

---------------------------------------------------------------------------

Tom Lane wrote:
> Michael Glaesemann <grzm@seespotcode.net> 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.099999999999999645
> (gdb) s
> 2575            result->day += (int32) month_remainder_days;
> (gdb) p month_remainder_days
> $20 = 2.9999999999999893
> 
> 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   bruce@momjian.us
  EnterpriseDB    http://www.enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +

Re: Interval aggregate regression failure (expected seems

Michael Glaesemann <grzm@seespotcode.net> — 2006-08-11T05:06:47Z

On Aug 11, 2006, at 13:48 , Bruce Momjian wrote:

>
> Have we made any progress on this?

I made a bit of progress but am still having issues when --enable- 
integer-datetimes is not enabled. I need to spend some time with gdb  
and figure out what's going on. I probably won't be able to get time  
to look at it until Aug 18.

Michael Glaesemann
grzm seespotcode net

Re: [HACKERS] Interval aggregate regression failure (expected

Bruce Momjian <bruce@momjian.us> — 2006-08-26T02:40:21Z

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 <grzm@seespotcode.net> 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.099999999999999645
> (gdb) s
> 2575            result->day += (int32) month_remainder_days;
> (gdb) p month_remainder_days
> $20 = 2.9999999999999893
> 
> 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   bruce@momjian.us
  EnterpriseDB    http://www.enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +

Re: [HACKERS] Interval aggregate regression failure (expected seems

Michael Glaesemann <grzm@seespotcode.net> — 2006-08-28T07:24:20Z

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.c	25 Jul 2006 03:51:21 -0000	1.169
--- src/backend/utils/adt/datetime.c	28 Aug 2006 07:08:46 -0000
***************
*** 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) * 1000000;
   #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) * 1000000;
   #else
! 								*fsec += fval - sec;
   #endif
+ 							}
   						}
   						tmask = (fmask & DTK_M(DAY)) ? 0 : DTK_M(DAY);
   						break;
***************
*** 2938,2953 ****
   						tm->tm_mon += val;
   						if (fval != 0)
   						{
! 							int			sec;
!
! 							fval *= DAYS_PER_MONTH * SECS_PER_DAY;
! 							sec = fval;
! 							tm->tm_sec += sec;
   #ifdef HAVE_INT64_TIMESTAMP
! 							*fsec += (fval - sec) * 1000000;
   #else
! 							*fsec += fval - sec;
   #endif
   						}
   						tmask = DTK_M(MONTH);
   						break;
--- 2945,2967 ----
   						tm->tm_mon += val;
   						if (fval != 0)
   						{
! 							int         day;
! 							fval *= DAYS_PER_MONTH;
! 							day = fval;
! 							tm->tm_mday += day;
! 							fval -= day;
! 							if (fval != 0)
! 							{
! 								int			sec;
! 								fval *= SECS_PER_DAY;
! 								sec = fval;
! 								tm->tm_sec += sec;
   #ifdef HAVE_INT64_TIMESTAMP
! 								*fsec += (fval - sec) * 1000000;
   #else
! 								*fsec += fval - sec;
   #endif
+ 							}
   						}
   						tmask = DTK_M(MONTH);
   						break;
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	28 Aug 2006 07:08:48 -0000
***************
*** 2547,2556 ****
   	Interval   *span = PG_GETARG_INTERVAL_P(0);
   	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)
--- 2547,2572 ----
   	Interval   *span = PG_GETARG_INTERVAL_P(0);
   	float8		factor = PG_GETARG_FLOAT8(1);
   	double		month_remainder,
+ 				month_remainder_usecs = 0.,
   				day_remainder,
! 				day_remainder_usecs = 0.;
! 	int32		extra_days;
   	Interval   *result;

+ /*
+    example a:
+    select '1.5 mon'::interval / 10;
+    span = { time = 0, day = 15, month = 1 }
+    factor = 10.0
+    result: 4 days 12:00 { time = 43200.0 (int64 43_200_000_000),  
day = 4,  month = 0 }
+
+    example b:
+    select '41 mon'::interval / 10;
+    span = { time = 0, day = 0, month = 41 }
+    factor = 10.0
+    result: 4 mon 3 days { time = 0, day = 3, month = 4 }
+  */
+
   	result = (Interval *) palloc(sizeof(Interval));

   	if (factor == 0.0)
***************
*** 2559,2584 ****
   				 errmsg("division by zero")));

   	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;

   	/*
! 	 * 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;

   #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);
--- 2575,2682 ----
   				 errmsg("division by zero")));

   	month_remainder = span->month / factor;
! 	/*
! 	   a: month_remainder = 1 / 10.0 = 0.1
! 	   b: month_remainder = 41 / 10.0 = 4.1
! 	 */
   	result->month = (int32) month_remainder;
! 	/*
! 	   a: result->month = 0
! 	   b: result->month = 4
! 	 */
! 	 /* FIXME integer overflow */
! 	month_remainder_usecs = rint( (double) (month_remainder - result- 
 >month)
! 										* USECS_PER_MONTH);
! 	/*
! 		a: month_remainder_usecs = rint((0.1 - 0) * ((double)  
DAYS_PER_MONTH * (double) USECS_PER_DAY))
! 								 = rint(0.1 * (30.0 * (SECS_PER_DAY * 1000000)))
! 								 = rint(0.1 * 30.0 * 86400 * 1000000)
! 								 = 259_200_000_000
! 		b: month_remainder_usecs = rint((4.1 - 4) * ((double)  
DAYS_PER_MONTH * (double) USECS_PER_DAY))
! 								 = rint( 0.1 * 30.0 * 86400 * 1000000)
! 								 = 259_200_000_000
! 	 */
!
! 	/*
! 	 * Due to the inherent inaccuracy of floating point division,  
round to
! 	 * microsecond accuracy. Scale up intermediate results to  
microseconds
! 	 * and scale back down for the final result.
! 	 */

+ 	day_remainder = span->day / factor;
   	/*
! 		a: day_remainder = 15 / 10.0 = 1.5
! 		b: day_remainder = 0 / 10.0 = 0.0
! 	 */
! 	result->day = (int32) day_remainder;
! 	/*
! 		a: result->day = (int32) 1.5 = 1
! 		b: result->day = (int32) 0.0 = 0
! 	 */
! 	 /* FIXME integer overflow */
! 	day_remainder_usecs = rint( (double) (day_remainder - result->day)  
* (double) USECS_PER_DAY);
! 	/*
! 		a: day_remainder_usecs = rint((1.5 - 1) * (SECS_PER_DAY * 1000000))
! 							   = rint( 0.5 * 86400 * 1_000_000 )
! 							   = 43_200_000_000
! 		b: day_remainder_usecs = rint((0.0 - 0 ) * 86400 * 1_000_000)
! 							   = 0
! 	 */
!
! 	/*
! 	 * Handle any fractional parts the same way as in interval_mul.
! 	 * Fractional months and fractional days are added to the result  
separately
! 	 * to prevent their time fractional components from contributing  
to the day
! 	 * component.
   	 */

   	/* fractional months full days into days */
! 	/* FIXME integer overflow */
! 	extra_days = (int32) (month_remainder_usecs / (double)  
USECS_PER_DAY);
! 	/*
! 		a: extra_days = (int32) (259_200_000_000 / (SECS_PER_DAY * 1000000))
! 					  = (int32) (259_200_000_000 / (86400 * 1000000))
! 					  = (int32) 3.0
! 					  = 3
! 		b: extra_days = (int32) (259_200_000_000 / (86400 * 1000000) )
! 					  = (int32) 3.0
! 					  = 3
! 	 */
! 	result->day += extra_days;
! 	/*
! 		a: result->day = 1 + 3 = 4
! 		b: result->day = 0 + 3 = 3
! 	 */
! 	 /* FIXME integer overflow */
! 	month_remainder_usecs -= extra_days * (double) USECS_PER_DAY;
! 	/*
! 		a: month_remainder_usecs = 259_200_000_000 - (3 * (86400 * 1000000))
! 								 = 259_200_000_000 - (3 * (86400 * 1000000))
! 								 = 259_200_000_000 - 259_200_000_000
! 								 = 0
! 		b: month_remainder_usecs = 259_200_000_000 - (3 * (86400 * 1000000))
! 								 = 259_200_000_000 - 259_200_000_000
! 								 = 0
! 	 */

   #ifdef HAVE_INT64_TIMESTAMP
! 	result->time = rint(span->time / factor +
! 							day_remainder_usecs + month_remainder_usecs);
! 	/*
! 		a: result->time = rint(0.0 / 10.0 + 43_200_000_000 + 0.0)
! 						= 43_200_000_000
! 		b: result->time = rint(0.0 / 10.0 + 0 + 0.0)
! 						= 0
! 	 */
   #else
! 	result->time = rint(span->time / factor * 1.0e6 * +
! 							day_remainder_usecs + month_remainder_usecs) / 1.0e6;
! 	/*
! 		a: result->time = rint(0.0 / 10.0 * 1.0e6 + 43_200_000_000 +  
0.0) / 1.0e6
! 						= 43200.0
! 		b: result->time = rint(0.0 / 10.0 * 1.0e6 + 0.0 + 0.0) / 1.0e6
! 						= 0.0
! 	 */
   #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	28 Aug 2006 07:08:51 -0000
***************
*** 79,92 ****
   #define SECS_PER_YEAR	(36525 * 864)	/* avoid floating-point  
computation */
   #define SECS_PER_DAY	86400
   #define SECS_PER_HOUR	3600
! #define SECS_PER_MINUTE 60
   #define MINS_PER_HOUR	60

   #ifdef HAVE_INT64_TIMESTAMP
   #define USECS_PER_DAY	INT64CONST(86400000000)
   #define USECS_PER_HOUR	INT64CONST(3600000000)
! #define USECS_PER_MINUTE INT64CONST(60000000)
   #define USECS_PER_SEC	INT64CONST(1000000)
   #endif

   /*
--- 79,96 ----
   #define SECS_PER_YEAR	(36525 * 864)	/* avoid floating-point  
computation */
   #define SECS_PER_DAY	86400
   #define SECS_PER_HOUR	3600
! #define SECS_PER_MINUTE	60
   #define MINS_PER_HOUR	60

   #ifdef HAVE_INT64_TIMESTAMP
+ #define USECS_PER_MONTH	INT64CONST(2592000000000)
   #define USECS_PER_DAY	INT64CONST(86400000000)
   #define USECS_PER_HOUR	INT64CONST(3600000000)
! #define USECS_PER_MINUTE	INT64CONST(60000000)
   #define USECS_PER_SEC	INT64CONST(1000000)
+ #else
+ #define USECS_PER_DAY	(SECS_PER_DAY * 1000000)
+ #define USECS_PER_MONTH ((double) DAYS_PER_MONTH * (double)  
USECS_PER_DAY)
   #endif

   /*

Re: [HACKERS] Interval aggregate regression failure (expected seems

Michael Glaesemann <grzm@seespotcode.net> — 2006-08-29T06:38:46Z

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 -0000	1.165
--- src/backend/utils/adt/timestamp.c	29 Aug 2006 06:20:03 -0000
***************
*** 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 / factor;
   	result->month = (int32) month_remainder;
- 	result->day = (int32) day_remainder;
   	month_remainder -= result->month;
- 	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;

   #ifdef HAVE_INT64_TIMESTAMP
   	result->time = rint(span->time / factor + day_remainder *  
USECS_PER_DAY);
--- 2598,2648 ----

   	month_remainder = span->month / factor;
   	day_remainder = span->day / factor;
+
   	result->month = (int32) month_remainder;
   	month_remainder -= result->month;

! 	result->day = (int32) day_remainder;
! 	day_remainder -= result->day;

   	/* 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;
!
!     /*
!      * Add to result->day any fractional days from the month  
component that
!      * round to 24 hour periods after being converted to time.
!      * Handle any fractional parts the same way as in interval_mul.
!      */
!
! #ifdef HAVE_INT64_TIMESTAMP
!     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);

Re: [HACKERS] Interval aggregate regression failure (expected seems

Michael Glaesemann <grzm@seespotcode.net> — 2006-08-29T15:02:41Z

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.

Thanks!

Michael Glaesemann
grzm seespotcode net

Re: [HACKERS] Interval aggregate regression failure

Bruce Momjian <bruce@momjian.us> — 2006-08-29T16:13:11Z

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.000001).

-- 
  Bruce Momjian   bruce@momjian.us
  EnterpriseDB    http://www.enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +

Re: [HACKERS] Interval aggregate regression failure (expected seems

Michael Glaesemann <grzm@seespotcode.net> — 2006-08-29T16:22:46Z

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

Re: [HACKERS] Interval aggregate regression failure

Bruce Momjian <bruce@momjian.us> — 2006-08-29T22:12:54Z

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.000001 of a whole number, it should be
rounded to a whole number. Patch attached with comments added.

-- 
  Bruce Momjian   bruce@momjian.us
  EnterpriseDB    http://www.enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +

Re: [HACKERS] Interval aggregate regression failure (expected seems

Michael Glaesemann <grzm@seespotcode.net> — 2006-08-30T01:30:43Z

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.000001 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.000001 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.000001 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
+ 		month_remainder_days = rint(month_remainder_days);

Anyway, I'll pound on this some more tonight.

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	30 Aug 2006 00:48:37 -0000
***************
*** 2518,2523 ****
--- 2518,2536 ----

   	/* 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.000001 of an integer, we round them to integers.
+ 	 */
+ 	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
+ 		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;
***************
*** 2571,2576 ****
--- 2584,2602 ----

   	/* 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.000001 of an integer, we round them to integers.
+ 	 */
+ 	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
+ 		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;

Re: [HACKERS] Interval aggregate regression failure

Bruce Momjian <bruce@momjian.us> — 2006-08-30T03:50:40Z

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.100000000000000
	2.999999999999989
	3.000000000000000


> 
> 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.000001 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.000001 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.000001 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);
> 

Good idea.  I updated the patch to do that.

> 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?

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.

> 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
> + 		month_remainder_days = rint(month_remainder_days);
> 
> Anyway, I'll pound on this some more tonight.

You don't want to do any conditional tests.  SECS_PER_DAY is all you
want.  Those two macros are useful only in representing the internal
storage format, not for float compuations of rounding.

Patch attached.  It also fixes a regression test output too.

-- 
  Bruce Momjian   bruce@momjian.us
  EnterpriseDB    http://www.enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +

Re: [HACKERS] Interval aggregate regression failure (expected seems

Michael Glaesemann <grzm@seespotcode.net> — 2006-08-30T05:44:42Z

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.100000000000000
> 	2.999999999999989
> 	3.000000000000000

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.100000
2.999999999999989
3.000000000000000
$

> 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

Re: [HACKERS] Interval aggregate regression failure (expected seems

Michael Glaesemann <grzm@seespotcode.net> — 2006-08-30T13:20:58Z

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

Re: [HACKERS] Interval aggregate regression failure

Bruce Momjian <bruce@momjian.us> — 2006-08-30T17:08:45Z

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   bruce@momjian.us
  EnterpriseDB    http://www.enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +

Re: [HACKERS] Interval aggregate regression failure

Bruce Momjian <bruce@momjian.us> — 2006-08-30T17:09:04Z

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   bruce@momjian.us
  EnterpriseDB    http://www.enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +

Re: [HACKERS] Interval aggregate regression failure

Tom Lane <tgl@sss.pgh.pa.us> — 2006-08-31T18:29:46Z

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

Re: [HACKERS] Interval aggregate regression failure

Bruce Momjian <bruce@momjian.us> — 2006-08-31T18:57:41Z

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. +

Re: [HACKERS] Interval aggregate regression failure

Tom Lane <tgl@sss.pgh.pa.us> — 2006-08-31T19:46:08Z

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

Re: [HACKERS] Interval aggregate regression failure

Bruce Momjian <bruce@momjian.us> — 2006-08-31T20:05:43Z

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. +

Re: [HACKERS] Interval aggregate regression failure

Michael Glaesemann <grzm@seespotcode.net> — 2006-08-31T23:56:28Z

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

Re: [HACKERS] Interval aggregate regression failure

Bruce Momjian <bruce@momjian.us> — 2006-09-01T02:03:20Z

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. +

Re: [HACKERS] Interval aggregate regression failure

Tom Lane <tgl@sss.pgh.pa.us> — 2006-09-01T02:14:36Z

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

Re: [HACKERS] Interval aggregate regression failure

Bruce Momjian <bruce@momjian.us> — 2006-09-01T02:31:10Z

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. +

Re: [HACKERS] Interval aggregate regression failure

Michael Glaesemann <grzm@seespotcode.net> — 2006-09-01T10:54:57Z

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

Re: [HACKERS] Interval aggregate regression failure

Michael Glaesemann <grzm@seespotcode.net> — 2006-09-01T11:01:22Z

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

Re: [HACKERS] Interval aggregate regression failure

Michael Glaesemann <grzm@seespotcode.net> — 2006-09-01T11:39:19Z

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))

Re: [HACKERS] Interval aggregate regression failure

Michael Glaesemann <grzm@seespotcode.net> — 2006-09-01T11:49:07Z

Please ignore the patch I just sent. Much too quick with the send  
button.

Michael Glaesemann
grzm seespotcode net

Re: [HACKERS] Interval aggregate regression failure

Tom Lane <tgl@sss.pgh.pa.us> — 2006-09-01T13:44:04Z

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

Re: [HACKERS] Interval aggregate regression failure

Michael Glaesemann <grzm@seespotcode.net> — 2006-09-01T13:54:39Z

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))