pgsql: Fix NUMERIC modulus to properly truncate division in computation.

Lists: pgsql-committerspgsql-hackers
From: momjian(at)svr1(dot)postgresql(dot)org (Bruce Momjian)
To: pgsql-committers(at)postgresql(dot)org
Subject: pgsql: Fix NUMERIC modulus to properly truncate division in computation.
Date: 2005-06-04 14:12:50
Message-ID: 20050604141250.C7A655292B@svr1.postgresql.org
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-committers pgsql-hackers

Log Message:
-----------
Fix NUMERIC modulus to properly truncate division in computation.
Division rounding was causing incorrect results. Test case:

test=> SELECT 12345678901234567890 % 123;
?column?
----------
78
(1 row)

Was returning -45.

Modified Files:
--------------
pgsql/src/backend/utils/adt:
numeric.c (r1.83 -> r1.84)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/adt/numeric.c.diff?r1=1.83&r2=1.84)


From: Christopher Kings-Lynne <chriskl(at)familyhealth(dot)com(dot)au>
To: Bruce Momjian <momjian(at)svr1(dot)postgresql(dot)org>
Cc: pgsql-committers(at)postgresql(dot)org
Subject: Re: pgsql: Fix NUMERIC modulus to properly truncate
Date: 2005-06-04 15:41:18
Message-ID: 42A1CB9E.7090400@familyhealth.com.au
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-committers pgsql-hackers

Is this a backport?

Bruce Momjian wrote:
> Log Message:
> -----------
> Fix NUMERIC modulus to properly truncate division in computation.
> Division rounding was causing incorrect results. Test case:
>
> test=> SELECT 12345678901234567890 % 123;
> ?column?
> ----------
> 78
> (1 row)
>
> Was returning -45.
>
> Modified Files:
> --------------
> pgsql/src/backend/utils/adt:
> numeric.c (r1.83 -> r1.84)
> (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/adt/numeric.c.diff?r1=1.83&r2=1.84)
>
> ---------------------------(end of broadcast)---------------------------
> TIP 3: if posting/reading through Usenet, please send an appropriate
> subscribe-nomail command to majordomo(at)postgresql(dot)org so that your
> message can get through to the mailing list cleanly


From: Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us>
To: Christopher Kings-Lynne <chriskl(at)familyhealth(dot)com(dot)au>
Cc: Bruce Momjian <momjian(at)svr1(dot)postgresql(dot)org>, pgsql-committers(at)postgresql(dot)org
Subject: Re: pgsql: Fix NUMERIC modulus to properly truncate
Date: 2005-06-04 15:45:46
Message-ID: 200506041545.j54FjkO19642@candle.pha.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-committers pgsql-hackers

Christopher Kings-Lynne wrote:
> Is this a backport?

No, I don't think so. It doesn't seem to be something that enough
people use to risk the change in behavior --- it might break something
that was working. But, if folks want it backported we can do it. It is
only a change to properly do modulus for numeric.

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

>
> Bruce Momjian wrote:
> > Log Message:
> > -----------
> > Fix NUMERIC modulus to properly truncate division in computation.
> > Division rounding was causing incorrect results. Test case:
> >
> > test=> SELECT 12345678901234567890 % 123;
> > ?column?
> > ----------
> > 78
> > (1 row)
> >
> > Was returning -45.
> >
> > Modified Files:
> > --------------
> > pgsql/src/backend/utils/adt:
> > numeric.c (r1.83 -> r1.84)
> > (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/adt/numeric.c.diff?r1=1.83&r2=1.84)
> >
> > ---------------------------(end of broadcast)---------------------------
> > TIP 3: if posting/reading through Usenet, please send an appropriate
> > subscribe-nomail command to majordomo(at)postgresql(dot)org so that your
> > message can get through to the mailing list cleanly
>
> ---------------------------(end of broadcast)---------------------------
> TIP 8: explain analyze is your friend
>

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


From: Christopher Kings-Lynne <chriskl(at)familyhealth(dot)com(dot)au>
To: Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us>
Cc: Bruce Momjian <momjian(at)svr1(dot)postgresql(dot)org>, pgsql-committers(at)postgresql(dot)org
Subject: Re: pgsql: Fix NUMERIC modulus to properly truncate
Date: 2005-06-05 04:02:07
Message-ID: 42A2793F.3020208@familyhealth.com.au
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-committers pgsql-hackers

> No, I don't think so. It doesn't seem to be something that enough
> people use to risk the change in behavior --- it might break something
> that was working. But, if folks want it backported we can do it. It is
> only a change to properly do modulus for numeric.

Well, from my point of view it's an absolute mathematical error - i'd
backport it. I can't see anyone relying on it :)

Chris


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Christopher Kings-Lynne <chriskl(at)familyhealth(dot)com(dot)au>
Cc: Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us>, pgsql-committers(at)postgresql(dot)org
Subject: Re: pgsql: Fix NUMERIC modulus to properly truncate
Date: 2005-06-05 04:51:43
Message-ID: 911.1117947103@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-committers pgsql-hackers

Christopher Kings-Lynne <chriskl(at)familyhealth(dot)com(dot)au> writes:
>> No, I don't think so. It doesn't seem to be something that enough
>> people use to risk the change in behavior --- it might break something
>> that was working. But, if folks want it backported we can do it. It is
>> only a change to properly do modulus for numeric.

> Well, from my point of view it's an absolute mathematical error - i'd
> backport it. I can't see anyone relying on it :)

Doesn't this patch break the basic theorem that

a = trunc(a / b) * b + (a mod b)

? If division rounds and mod doesn't, you've got pretty serious issues.

regards, tom lane


From: Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Christopher Kings-Lynne <chriskl(at)familyhealth(dot)com(dot)au>, PostgreSQL-development <pgsql-hackers(at)postgreSQL(dot)org>
Subject: Re: [COMMITTERS] pgsql: Fix NUMERIC modulus to properly truncate
Date: 2005-06-06 15:25:07
Message-ID: 200506061525.j56FP7j13002@candle.pha.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-committers pgsql-hackers

Tom Lane wrote:
> Christopher Kings-Lynne <chriskl(at)familyhealth(dot)com(dot)au> writes:
> >> No, I don't think so. It doesn't seem to be something that enough
> >> people use to risk the change in behavior --- it might break something
> >> that was working. But, if folks want it backported we can do it. It is
> >> only a change to properly do modulus for numeric.
>
> > Well, from my point of view it's an absolute mathematical error - i'd
> > backport it. I can't see anyone relying on it :)
>
> Doesn't this patch break the basic theorem that
>
> a = trunc(a / b) * b + (a mod b)
>
> ? If division rounds and mod doesn't, you've got pretty serious issues.

Well, this is a good question. In the equation above we assume '/' is
an integer division. The problem with NUMERIC when used with zero-scale
operands is that the result is already _rounded_ to the nearest hole
number before it gets to trunc(), and that is why we used to get
negative modulus values. I assume the big point is that we don't offer
any way for users to get a NUMERIC division without rounding.

With integers, we always round down to the nearest whole number on
division; float doesn't offer a modulus operator, and C doesn't support
it either.

We round NUMERICs to the specific scale because we want to give the most
accurate value:

test=> select 100000000000000000000000::numeric(24,0) /
11::numeric(24,0);
?column?
------------------------
9090909090909090909091

The actual values is:
--
9090909090909090909090.90

But the problem is that the equation at the top assumes the division is
not rounded. Should we supply a NUMERIC division operator that doesn't
round? integer doesn't need it, and float doesn't have the accurate
precision needed for modulus operators. The user could supply some
digits in the division:

test=> select 100000000000000000000000::numeric(30,6) /
11::numeric(24,0);
?column?
-------------------------------
9090909090909090909090.909091
(1 row)

but there really is no _right_ value to prevent rounding (think
0.9999999). A non-rounding NUMERIC division would require duplicating
numeric_div() but with a false for 'round', and adding operators.

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


From: Paul Tillotson <pntil(at)shentel(dot)net>
To: Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Christopher Kings-Lynne <chriskl(at)familyhealth(dot)com(dot)au>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [COMMITTERS] pgsql: Fix NUMERIC modulus to properly
Date: 2005-06-07 01:26:21
Message-ID: 42A4F7BD.4060303@shentel.net
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-committers pgsql-hackers

Bruce Momjian wrote:

>Tom Lane wrote:
>
>
>>Christopher Kings-Lynne <chriskl(at)familyhealth(dot)com(dot)au> writes:
>>
>>
>>>>No, I don't think so. It doesn't seem to be something that enough
>>>>people use to risk the change in behavior --- it might break something
>>>>that was working. But, if folks want it backported we can do it. It is
>>>>only a change to properly do modulus for numeric.
>>>>
>>>>
>>>Well, from my point of view it's an absolute mathematical error - i'd
>>>backport it. I can't see anyone relying on it :)
>>>
>>>
>>Doesn't this patch break the basic theorem that
>>
>> a = trunc(a / b) * b + (a mod b)
>>
>>? If division rounds and mod doesn't, you've got pretty serious issues.
>>
>>
>
>Well, this is a good question. In the equation above we assume '/' is
>an integer division. The problem with NUMERIC when used with zero-scale
>operands is that the result is already _rounded_ to the nearest hole
>number before it gets to trunc(), and that is why we used to get
>negative modulus values. I assume the big point is that we don't offer
>any way for users to get a NUMERIC division without rounding.
>
>With integers, we always round down to the nearest whole number on
>division; float doesn't offer a modulus operator, and C doesn't support
>it either.
>
>We round NUMERICs to the specific scale because we want to give the most
>accurate value:
>
> test=> select 100000000000000000000000::numeric(24,0) /
> 11::numeric(24,0);
> ?column?
> ------------------------
> 9090909090909090909091
>
>The actual values is:
> --
> 9090909090909090909090.90
>
>But the problem is that the equation at the top assumes the division is
>not rounded. Should we supply a NUMERIC division operator that doesn't
>round? integer doesn't need it, and float doesn't have the accurate
>precision needed for modulus operators. The user could supply some
>digits in the division:
>
> test=> select 100000000000000000000000::numeric(30,6) /
> 11::numeric(24,0);
> ?column?
> -------------------------------
> 9090909090909090909090.909091
> (1 row)
>
>but there really is no _right_ value to prevent rounding (think
>0.9999999). A non-rounding NUMERIC division would require duplicating
>numeric_div() but with a false for 'round', and adding operators.
>
>
>
I would prefer that division didn't round, as with integers. You can
always calculate your result to 1 more decimal place and then round, but
there is no way to unround a rounded result.

Tom had asked whether PG passed the regression tests if we change the
round_var() to a trunc_var() at the end of the function div_var().

It does not pass, but I think that is because the regression test is
expecting that division will round up. (Curiously, the regression test
for "numeric" passes, but the regression test for aggregation--sum() I
think--is the one that fails.) I have attached the diffs here if anyone
is interested.

Regards,
Paul Tillotson

Attachment Content-Type Size
regression.diffs text/plain 628 bytes
regression.out text/plain 3.1 KB

From: Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us>
To: Paul Tillotson <pntil(at)shentel(dot)net>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Christopher Kings-Lynne <chriskl(at)familyhealth(dot)com(dot)au>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [COMMITTERS] pgsql: Fix NUMERIC modulus to properly truncate
Date: 2005-06-15 01:37:50
Message-ID: 200506150137.j5F1boe14413@candle.pha.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-committers pgsql-hackers


Have we made any decision on whether the old/new NUMERIC %(mod) code was
correct, and now to handle rounding? Should we offer a non-rounding
division operator for NUMERIC?

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

Paul Tillotson wrote:
> Bruce Momjian wrote:
>
> >Tom Lane wrote:
> >
> >
> >>Christopher Kings-Lynne <chriskl(at)familyhealth(dot)com(dot)au> writes:
> >>
> >>
> >>>>No, I don't think so. It doesn't seem to be something that enough
> >>>>people use to risk the change in behavior --- it might break something
> >>>>that was working. But, if folks want it backported we can do it. It is
> >>>>only a change to properly do modulus for numeric.
> >>>>
> >>>>
> >>>Well, from my point of view it's an absolute mathematical error - i'd
> >>>backport it. I can't see anyone relying on it :)
> >>>
> >>>
> >>Doesn't this patch break the basic theorem that
> >>
> >> a = trunc(a / b) * b + (a mod b)
> >>
> >>? If division rounds and mod doesn't, you've got pretty serious issues.
> >>
> >>
> >
> >Well, this is a good question. In the equation above we assume '/' is
> >an integer division. The problem with NUMERIC when used with zero-scale
> >operands is that the result is already _rounded_ to the nearest hole
> >number before it gets to trunc(), and that is why we used to get
> >negative modulus values. I assume the big point is that we don't offer
> >any way for users to get a NUMERIC division without rounding.
> >
> >With integers, we always round down to the nearest whole number on
> >division; float doesn't offer a modulus operator, and C doesn't support
> >it either.
> >
> >We round NUMERICs to the specific scale because we want to give the most
> >accurate value:
> >
> > test=> select 100000000000000000000000::numeric(24,0) /
> > 11::numeric(24,0);
> > ?column?
> > ------------------------
> > 9090909090909090909091
> >
> >The actual values is:
> > --
> > 9090909090909090909090.90
> >
> >But the problem is that the equation at the top assumes the division is
> >not rounded. Should we supply a NUMERIC division operator that doesn't
> >round? integer doesn't need it, and float doesn't have the accurate
> >precision needed for modulus operators. The user could supply some
> >digits in the division:
> >
> > test=> select 100000000000000000000000::numeric(30,6) /
> > 11::numeric(24,0);
> > ?column?
> > -------------------------------
> > 9090909090909090909090.909091
> > (1 row)
> >
> >but there really is no _right_ value to prevent rounding (think
> >0.9999999). A non-rounding NUMERIC division would require duplicating
> >numeric_div() but with a false for 'round', and adding operators.
> >
> >
> >
> I would prefer that division didn't round, as with integers. You can
> always calculate your result to 1 more decimal place and then round, but
> there is no way to unround a rounded result.
>
> Tom had asked whether PG passed the regression tests if we change the
> round_var() to a trunc_var() at the end of the function div_var().
>
> It does not pass, but I think that is because the regression test is
> expecting that division will round up. (Curiously, the regression test
> for "numeric" passes, but the regression test for aggregation--sum() I
> think--is the one that fails.) I have attached the diffs here if anyone
> is interested.
>
> Regards,
> Paul Tillotson
>

> *** ./expected/aggregates.out Sun May 29 19:58:43 2005
> --- ./results/aggregates.out Mon Jun 6 21:01:11 2005
> ***************
> *** 10,16 ****
> SELECT avg(a) AS avg_32 FROM aggtest WHERE a < 100;
> avg_32
> ---------------------
> ! 32.6666666666666667
> (1 row)
>
> -- In 7.1, avg(float4) is computed using float8 arithmetic.
> --- 10,16 ----
> SELECT avg(a) AS avg_32 FROM aggtest WHERE a < 100;
> avg_32
> ---------------------
> ! 32.6666666666666666
> (1 row)
>
> -- In 7.1, avg(float4) is computed using float8 arithmetic.
>
> ======================================================================
>

> test boolean ... ok
> test char ... ok
> test name ... ok
> test varchar ... ok
> test text ... ok
> test int2 ... ok
> test int4 ... ok
> test int8 ... ok
> test oid ... ok
> test float4 ... ok
> test float8 ... ok
> test bit ... ok
> test numeric ... ok
> test strings ... ok
> test numerology ... ok
> test point ... ok
> test lseg ... ok
> test box ... ok
> test path ... ok
> test polygon ... ok
> test circle ... ok
> test date ... ok
> test time ... ok
> test timetz ... ok
> test timestamp ... ok
> test timestamptz ... ok
> test interval ... ok
> test abstime ... ok
> test reltime ... ok
> test tinterval ... ok
> test inet ... ok
> test comments ... ok
> test oidjoins ... ok
> test type_sanity ... ok
> test opr_sanity ... ok
> test geometry ... ok
> test horology ... ok
> test insert ... ok
> test create_function_1 ... ok
> test create_type ... ok
> test create_table ... ok
> test create_function_2 ... ok
> test copy ... ok
> test constraints ... ok
> test triggers ... ok
> test create_misc ... ok
> test create_aggregate ... ok
> test create_operator ... ok
> test create_index ... ok
> test inherit ... ok
> test vacuum ... ok
> test create_view ... ok
> test sanity_check ... ok
> test errors ... ok
> test select ... ok
> test select_into ... ok
> test select_distinct ... ok
> test select_distinct_on ... ok
> test select_implicit ... ok
> test select_having ... ok
> test subselect ... ok
> test union ... ok
> test case ... ok
> test join ... ok
> test aggregates ... FAILED
> test transactions ... ok
> test random ... ok
> test portals ... ok
> test arrays ... ok
> test btree_index ... ok
> test hash_index ... ok
> test update ... ok
> test namespace ... ok
> test privileges ... ok
> test misc ... ok
> test select_views ... ok
> test portals_p2 ... ok
> test rules ... ok
> test foreign_key ... ok
> test cluster ... ok
> test limit ... ok
> test plpgsql ... ok
> test copy2 ... ok
> test temp ... ok
> test domain ... ok
> test rangefuncs ... ok
> test prepare ... ok
> test without_oid ... ok
> test conversion ... ok
> test truncate ... ok
> test alter_table ... ok
> test sequence ... ok
> test polymorphism ... ok
> test rowtypes ... ok
> test stats ... ok
> test tablespace ... ok

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


From: Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us>
To: Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us>
Cc: Paul Tillotson <pntil(at)shentel(dot)net>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Christopher Kings-Lynne <chriskl(at)familyhealth(dot)com(dot)au>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [COMMITTERS] pgsql: Fix NUMERIC modulus to properly truncate
Date: 2005-06-25 01:32:29
Message-ID: 200506250132.j5P1WTq29384@candle.pha.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-committers pgsql-hackers


With no conclusion on this, I have added a TODO item:

* Add NUMERIC division operator that doesn't round?

Currently NUMERIC _rounds_ the result to the specified precision.
This means division can return a result that multiplied by the
divisor is greater than the dividend, e.g. this returns a value > 10:

SELECT (10::numeric(2,0) / 6::numeric(2,0))::numeric(2,0) * 6;

The positive modulus result returned by NUMERICs might be considered
inaccurate, in one sense.

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

Bruce Momjian wrote:
>
> Have we made any decision on whether the old/new NUMERIC %(mod) code was
> correct, and now to handle rounding? Should we offer a non-rounding
> division operator for NUMERIC?
>
> ---------------------------------------------------------------------------
>
> Paul Tillotson wrote:
> > Bruce Momjian wrote:
> >
> > >Tom Lane wrote:
> > >
> > >
> > >>Christopher Kings-Lynne <chriskl(at)familyhealth(dot)com(dot)au> writes:
> > >>
> > >>
> > >>>>No, I don't think so. It doesn't seem to be something that enough
> > >>>>people use to risk the change in behavior --- it might break something
> > >>>>that was working. But, if folks want it backported we can do it. It is
> > >>>>only a change to properly do modulus for numeric.
> > >>>>
> > >>>>
> > >>>Well, from my point of view it's an absolute mathematical error - i'd
> > >>>backport it. I can't see anyone relying on it :)
> > >>>
> > >>>
> > >>Doesn't this patch break the basic theorem that
> > >>
> > >> a = trunc(a / b) * b + (a mod b)
> > >>
> > >>? If division rounds and mod doesn't, you've got pretty serious issues.
> > >>
> > >>
> > >
> > >Well, this is a good question. In the equation above we assume '/' is
> > >an integer division. The problem with NUMERIC when used with zero-scale
> > >operands is that the result is already _rounded_ to the nearest hole
> > >number before it gets to trunc(), and that is why we used to get
> > >negative modulus values. I assume the big point is that we don't offer
> > >any way for users to get a NUMERIC division without rounding.
> > >
> > >With integers, we always round down to the nearest whole number on
> > >division; float doesn't offer a modulus operator, and C doesn't support
> > >it either.
> > >
> > >We round NUMERICs to the specific scale because we want to give the most
> > >accurate value:
> > >
> > > test=> select 100000000000000000000000::numeric(24,0) /
> > > 11::numeric(24,0);
> > > ?column?
> > > ------------------------
> > > 9090909090909090909091
> > >
> > >The actual values is:
> > > --
> > > 9090909090909090909090.90
> > >
> > >But the problem is that the equation at the top assumes the division is
> > >not rounded. Should we supply a NUMERIC division operator that doesn't
> > >round? integer doesn't need it, and float doesn't have the accurate
> > >precision needed for modulus operators. The user could supply some
> > >digits in the division:
> > >
> > > test=> select 100000000000000000000000::numeric(30,6) /
> > > 11::numeric(24,0);
> > > ?column?
> > > -------------------------------
> > > 9090909090909090909090.909091
> > > (1 row)
> > >
> > >but there really is no _right_ value to prevent rounding (think
> > >0.9999999). A non-rounding NUMERIC division would require duplicating
> > >numeric_div() but with a false for 'round', and adding operators.
> > >
> > >
> > >
> > I would prefer that division didn't round, as with integers. You can
> > always calculate your result to 1 more decimal place and then round, but
> > there is no way to unround a rounded result.
> >
> > Tom had asked whether PG passed the regression tests if we change the
> > round_var() to a trunc_var() at the end of the function div_var().
> >
> > It does not pass, but I think that is because the regression test is
> > expecting that division will round up. (Curiously, the regression test
> > for "numeric" passes, but the regression test for aggregation--sum() I
> > think--is the one that fails.) I have attached the diffs here if anyone
> > is interested.
> >
> > Regards,
> > Paul Tillotson
> >
>
> > *** ./expected/aggregates.out Sun May 29 19:58:43 2005
> > --- ./results/aggregates.out Mon Jun 6 21:01:11 2005
> > ***************
> > *** 10,16 ****
> > SELECT avg(a) AS avg_32 FROM aggtest WHERE a < 100;
> > avg_32
> > ---------------------
> > ! 32.6666666666666667
> > (1 row)
> >
> > -- In 7.1, avg(float4) is computed using float8 arithmetic.
> > --- 10,16 ----
> > SELECT avg(a) AS avg_32 FROM aggtest WHERE a < 100;
> > avg_32
> > ---------------------
> > ! 32.6666666666666666
> > (1 row)
> >
> > -- In 7.1, avg(float4) is computed using float8 arithmetic.
> >
> > ======================================================================
> >
>
> > test boolean ... ok
> > test char ... ok
> > test name ... ok
> > test varchar ... ok
> > test text ... ok
> > test int2 ... ok
> > test int4 ... ok
> > test int8 ... ok
> > test oid ... ok
> > test float4 ... ok
> > test float8 ... ok
> > test bit ... ok
> > test numeric ... ok
> > test strings ... ok
> > test numerology ... ok
> > test point ... ok
> > test lseg ... ok
> > test box ... ok
> > test path ... ok
> > test polygon ... ok
> > test circle ... ok
> > test date ... ok
> > test time ... ok
> > test timetz ... ok
> > test timestamp ... ok
> > test timestamptz ... ok
> > test interval ... ok
> > test abstime ... ok
> > test reltime ... ok
> > test tinterval ... ok
> > test inet ... ok
> > test comments ... ok
> > test oidjoins ... ok
> > test type_sanity ... ok
> > test opr_sanity ... ok
> > test geometry ... ok
> > test horology ... ok
> > test insert ... ok
> > test create_function_1 ... ok
> > test create_type ... ok
> > test create_table ... ok
> > test create_function_2 ... ok
> > test copy ... ok
> > test constraints ... ok
> > test triggers ... ok
> > test create_misc ... ok
> > test create_aggregate ... ok
> > test create_operator ... ok
> > test create_index ... ok
> > test inherit ... ok
> > test vacuum ... ok
> > test create_view ... ok
> > test sanity_check ... ok
> > test errors ... ok
> > test select ... ok
> > test select_into ... ok
> > test select_distinct ... ok
> > test select_distinct_on ... ok
> > test select_implicit ... ok
> > test select_having ... ok
> > test subselect ... ok
> > test union ... ok
> > test case ... ok
> > test join ... ok
> > test aggregates ... FAILED
> > test transactions ... ok
> > test random ... ok
> > test portals ... ok
> > test arrays ... ok
> > test btree_index ... ok
> > test hash_index ... ok
> > test update ... ok
> > test namespace ... ok
> > test privileges ... ok
> > test misc ... ok
> > test select_views ... ok
> > test portals_p2 ... ok
> > test rules ... ok
> > test foreign_key ... ok
> > test cluster ... ok
> > test limit ... ok
> > test plpgsql ... ok
> > test copy2 ... ok
> > test temp ... ok
> > test domain ... ok
> > test rangefuncs ... ok
> > test prepare ... ok
> > test without_oid ... ok
> > test conversion ... ok
> > test truncate ... ok
> > test alter_table ... ok
> > test sequence ... ok
> > test polymorphism ... ok
> > test rowtypes ... ok
> > test stats ... ok
> > test tablespace ... ok
>
> --
> Bruce Momjian | http://candle.pha.pa.us
> pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
> + If your life is a hard drive, | 13 Roberts Road
> + Christ can be your backup. | Newtown Square, Pennsylvania 19073
>
> ---------------------------(end of broadcast)---------------------------
> TIP 7: don't forget to increase your free space map settings
>

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


From: Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us>
To: Paul Tillotson <pntil(at)shentel(dot)net>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Christopher Kings-Lynne <chriskl(at)familyhealth(dot)com(dot)au>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [COMMITTERS] pgsql: Fix NUMERIC modulus to properly
Date: 2005-06-25 02:14:09
Message-ID: 200506250214.j5P2E9103265@candle.pha.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-committers pgsql-hackers


I don't think we can justify having NUMERIC division default to
truncation, especially since most division has non-zero decimal digits.

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

Paul Tillotson wrote:
> Bruce Momjian wrote:
>
> >Tom Lane wrote:
> >
> >
> >>Christopher Kings-Lynne <chriskl(at)familyhealth(dot)com(dot)au> writes:
> >>
> >>
> >>>>No, I don't think so. It doesn't seem to be something that enough
> >>>>people use to risk the change in behavior --- it might break something
> >>>>that was working. But, if folks want it backported we can do it. It is
> >>>>only a change to properly do modulus for numeric.
> >>>>
> >>>>
> >>>Well, from my point of view it's an absolute mathematical error - i'd
> >>>backport it. I can't see anyone relying on it :)
> >>>
> >>>
> >>Doesn't this patch break the basic theorem that
> >>
> >> a = trunc(a / b) * b + (a mod b)
> >>
> >>? If division rounds and mod doesn't, you've got pretty serious issues.
> >>
> >>
> >
> >Well, this is a good question. In the equation above we assume '/' is
> >an integer division. The problem with NUMERIC when used with zero-scale
> >operands is that the result is already _rounded_ to the nearest hole
> >number before it gets to trunc(), and that is why we used to get
> >negative modulus values. I assume the big point is that we don't offer
> >any way for users to get a NUMERIC division without rounding.
> >
> >With integers, we always round down to the nearest whole number on
> >division; float doesn't offer a modulus operator, and C doesn't support
> >it either.
> >
> >We round NUMERICs to the specific scale because we want to give the most
> >accurate value:
> >
> > test=> select 100000000000000000000000::numeric(24,0) /
> > 11::numeric(24,0);
> > ?column?
> > ------------------------
> > 9090909090909090909091
> >
> >The actual values is:
> > --
> > 9090909090909090909090.90
> >
> >But the problem is that the equation at the top assumes the division is
> >not rounded. Should we supply a NUMERIC division operator that doesn't
> >round? integer doesn't need it, and float doesn't have the accurate
> >precision needed for modulus operators. The user could supply some
> >digits in the division:
> >
> > test=> select 100000000000000000000000::numeric(30,6) /
> > 11::numeric(24,0);
> > ?column?
> > -------------------------------
> > 9090909090909090909090.909091
> > (1 row)
> >
> >but there really is no _right_ value to prevent rounding (think
> >0.9999999). A non-rounding NUMERIC division would require duplicating
> >numeric_div() but with a false for 'round', and adding operators.
> >
> >
> >
> I would prefer that division didn't round, as with integers. You can
> always calculate your result to 1 more decimal place and then round, but
> there is no way to unround a rounded result.
>
> Tom had asked whether PG passed the regression tests if we change the
> round_var() to a trunc_var() at the end of the function div_var().
>
> It does not pass, but I think that is because the regression test is
> expecting that division will round up. (Curiously, the regression test
> for "numeric" passes, but the regression test for aggregation--sum() I
> think--is the one that fails.) I have attached the diffs here if anyone
> is interested.
>
> Regards,
> Paul Tillotson
>

> *** ./expected/aggregates.out Sun May 29 19:58:43 2005
> --- ./results/aggregates.out Mon Jun 6 21:01:11 2005
> ***************
> *** 10,16 ****
> SELECT avg(a) AS avg_32 FROM aggtest WHERE a < 100;
> avg_32
> ---------------------
> ! 32.6666666666666667
> (1 row)
>
> -- In 7.1, avg(float4) is computed using float8 arithmetic.
> --- 10,16 ----
> SELECT avg(a) AS avg_32 FROM aggtest WHERE a < 100;
> avg_32
> ---------------------
> ! 32.6666666666666666
> (1 row)
>
> -- In 7.1, avg(float4) is computed using float8 arithmetic.
>
> ======================================================================
>

> test boolean ... ok
> test char ... ok
> test name ... ok
> test varchar ... ok
> test text ... ok
> test int2 ... ok
> test int4 ... ok
> test int8 ... ok
> test oid ... ok
> test float4 ... ok
> test float8 ... ok
> test bit ... ok
> test numeric ... ok
> test strings ... ok
> test numerology ... ok
> test point ... ok
> test lseg ... ok
> test box ... ok
> test path ... ok
> test polygon ... ok
> test circle ... ok
> test date ... ok
> test time ... ok
> test timetz ... ok
> test timestamp ... ok
> test timestamptz ... ok
> test interval ... ok
> test abstime ... ok
> test reltime ... ok
> test tinterval ... ok
> test inet ... ok
> test comments ... ok
> test oidjoins ... ok
> test type_sanity ... ok
> test opr_sanity ... ok
> test geometry ... ok
> test horology ... ok
> test insert ... ok
> test create_function_1 ... ok
> test create_type ... ok
> test create_table ... ok
> test create_function_2 ... ok
> test copy ... ok
> test constraints ... ok
> test triggers ... ok
> test create_misc ... ok
> test create_aggregate ... ok
> test create_operator ... ok
> test create_index ... ok
> test inherit ... ok
> test vacuum ... ok
> test create_view ... ok
> test sanity_check ... ok
> test errors ... ok
> test select ... ok
> test select_into ... ok
> test select_distinct ... ok
> test select_distinct_on ... ok
> test select_implicit ... ok
> test select_having ... ok
> test subselect ... ok
> test union ... ok
> test case ... ok
> test join ... ok
> test aggregates ... FAILED
> test transactions ... ok
> test random ... ok
> test portals ... ok
> test arrays ... ok
> test btree_index ... ok
> test hash_index ... ok
> test update ... ok
> test namespace ... ok
> test privileges ... ok
> test misc ... ok
> test select_views ... ok
> test portals_p2 ... ok
> test rules ... ok
> test foreign_key ... ok
> test cluster ... ok
> test limit ... ok
> test plpgsql ... ok
> test copy2 ... ok
> test temp ... ok
> test domain ... ok
> test rangefuncs ... ok
> test prepare ... ok
> test without_oid ... ok
> test conversion ... ok
> test truncate ... ok
> test alter_table ... ok
> test sequence ... ok
> test polymorphism ... ok
> test rowtypes ... ok
> test stats ... ok
> test tablespace ... ok

>
> ---------------------------(end of broadcast)---------------------------
> TIP 3: if posting/reading through Usenet, please send an appropriate
> subscribe-nomail command to majordomo(at)postgresql(dot)org so that your
> message can get through to the mailing list cleanly

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