Fwd: Re: [BUGS] dividing money by money
Hi Andy,
Do you want to package this up as a patch for 9.1? If not, is it OK
if I do?
-Kevin
Andy Balholm <andy@balholm.com> wrote:
Show quoted text
On Apr 1, 2010, at 7:57 AM, Kevin Grittner wrote:
I'm inclined to think it's better to have an explicit cast from
money to numeric, as long as it is exact, and leave the division
of money by money as float8. It does sort of beg the question of
whether we should support a cast back in the other direction,
though. I think that would wrap this all up in a tidy package.OK. Here is the whole thing in C:
[implementation as user-side functions]
I'm not quite sure how to go about changing it from an add-on function to a built-in one. So if you want to do that, go ahead. If you'd rather I did, just tell me how it's done.
Andy Balholm
(509) 276-2065
andy@balholm.com
On May 26, 2010, at 11:18 AM, Kevin Grittner wrote:
Show quoted text
Hi Andy,
Do you want to package this up as a patch for 9.1? If not, is it OK
if I do?-Kevin
Andy Balholm wrote:
On May 26, 2010, at 11:18 AM, Kevin Grittner wrote:
Do you want to package this up as a patch for 9.1? If not, is it
OK if I do?
I'm not quite sure how to go about changing it from an add-on
function to a built-in one. So if you want to do that, go ahead. If
you'd rather I did, just tell me how it's done.
You would basically move the functions and their prototypes to cash.c
and cash.h, and then (instead of CREATE FUNCTION, etc.) add
corresponding entries to pg_proc.h and pg_operator.h. (If I'm
missing something, someone please jump in.) Of course there's the
issue of adding the new operators to the documentation, too.
You would then generate a diff in context format and post to the
-hackers list with that file as an attachment. Don't forget to add
it to the "CommitFest" page:
https://commitfest.postgresql.org/action/commitfest_view/open
If you're going to do this, be sure to breeze through the developer's
FAQ:
http://wiki.postgresql.org/wiki/Developer_FAQ
Of course, if that's all too daunting, I can do it, but it seems to
me that you've already done the hard part (in creating working
functions), so I figured it might be satisfying for you to see it
through to the end.
-Kevin
Import Notes
Resolved by subject fallback
On May 30, 2010, at 6:53 AM, Kevin Grittner wrote:
You would basically move the functions and their prototypes to cash.c
and cash.h, and then (instead of CREATE FUNCTION, etc.) add
corresponding entries to pg_proc.h and pg_operator.h. (If I'm
missing something, someone please jump in.) Of course there's the
issue of adding the new operators to the documentation, too.
How do I come up with OID numbers for my new operators and functions?
Andy Balholm <andy@balholm.com> writes:
How do I come up with OID numbers for my new operators and functions?
Go into src/include/catalog and run the "unused_oids" script found
there. Pick some.
Your chances of getting numbers close to those currently in use for
money-related functions are probably nil, so I'd suggest just using
a consecutive range at or a bit above the last one currently in use.
regards, tom lane
On May 30, 2010, at 6:53 AM, Kevin Grittner wrote:
You would then generate a diff in context format and post to the
-hackers list with that file as an attachment.
Here it is:
Attachments:
dividing-money.diffapplication/octet-stream; name=dividing-money.diffDownload
*** ./doc/src/sgml/datatype.sgml.orig 2010-05-31 14:51:02.000000000 -0700
--- ./doc/src/sgml/datatype.sgml 2010-05-31 14:54:02.000000000 -0700
***************
*** 843,863 ****
floating-point literals, as well as typical
currency formatting, such as <literal>'$1,000.00'</literal>.
Output is generally in the latter form but depends on the locale.
! Non-quoted numeric values can be converted to <type>money</type> by
! casting the numeric value to <type>text</type> and then
! <type>money</type>, for example:
! <programlisting>
! SELECT 1234::text::money;
! </programlisting>
! There is no simple way of doing the reverse in a locale-independent
! manner, namely casting a <type>money</type> value to a numeric type.
! If you know the currency symbol and thousands separator you can use
! <function>regexp_replace()</>:
<programlisting>
! SELECT regexp_replace('52093.89'::money::text, '[$,]', '', 'g')::numeric;
</programlisting>
</para>
<para>
Since the output of this data type is locale-sensitive, it might not
--- 843,871 ----
floating-point literals, as well as typical
currency formatting, such as <literal>'$1,000.00'</literal>.
Output is generally in the latter form but depends on the locale.
! </para>
!
! <para>
! Values of the <type>numeric</type> data type can be cast to <type>money</type>.
! Other numeric types can be converted to <type>money</type> by casting to
! <type>numeric</type> first, for example:
! <programlisting>
! SELECT 1234::numeric::money;
! </programlisting>
! A <type>money</type> value can be cast to <type>numeric</type> without
! loss of precision. Conversion to other types could potentially lose precision,
! and it must be done in two stages:
<programlisting>
! SELECT '52093.89'::money::numeric::float;
</programlisting>
</para>
+
+ <para>
+ When a <type>money</type> value is divided by another <type>money</type> value,
+ the result is <type>double precision</type> (i.e. a pure number, not money);
+ the currency units cancel each other out in the division.
+ </para>
<para>
Since the output of this data type is locale-sensitive, it might not
*** ./src/backend/utils/adt/cash.c.orig 2010-05-31 14:51:28.000000000 -0700
--- ./src/backend/utils/adt/cash.c 2010-05-31 14:54:02.000000000 -0700
***************
*** 27,32 ****
--- 27,33 ----
#include "utils/builtins.h"
#include "utils/cash.h"
#include "utils/pg_locale.h"
+ #include "utils/numeric.h"
#define CASH_BUFSZ 36
***************
*** 845,847 ****
--- 846,944 ----
/* return as text datum */
PG_RETURN_TEXT_P(cstring_to_text(buf));
}
+
+ /*
+ * The functions cash_div_cash(), cash_numeric(), and numeric_cash()
+ * were written by Andy Balholm <andy@balholm.com>.
+ */
+
+ /* cash_div_cash()
+ * Divide cash by cash, returning float8.
+ */
+ Datum
+ cash_div_cash(PG_FUNCTION_ARGS)
+ {
+ Cash dividend = PG_GETARG_CASH(0);
+ Cash divisor = PG_GETARG_CASH(1);
+ float8 quotient;
+
+ if (divisor == 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_DIVISION_BY_ZERO),
+ errmsg("division by zero")));
+
+ quotient = (float8)dividend / (float8)divisor;
+ PG_RETURN_FLOAT8(quotient);
+ }
+
+ /* cash_numeric()
+ * Convert cash to numeric.
+ */
+ Datum
+ cash_numeric(PG_FUNCTION_ARGS)
+ {
+ Cash money = PG_GETARG_CASH(0);
+ int fpoint;
+ int64 scale;
+ int i;
+ Numeric result;
+ Datum amount;
+ Datum numeric_scale;
+ Datum one;
+
+ struct lconv *lconvert = PGLC_localeconv();
+
+ /*
+ * Find the number of digits after the decimal point.
+ * (These lines were copied from cash_in().)
+ */
+ fpoint = lconvert->frac_digits;
+ if (fpoint < 0 || fpoint > 10)
+ fpoint = 2;
+ scale = 1;
+ for (i = 0; i < fpoint; i++)
+ scale *= 10;
+
+ amount = DirectFunctionCall1(&int8_numeric, Int64GetDatum(money));
+ one = DirectFunctionCall1(&int8_numeric, Int64GetDatum(1));
+ numeric_scale = DirectFunctionCall1(&int8_numeric, Int64GetDatum(scale));
+ numeric_scale = DirectFunctionCall2(&numeric_div, one, numeric_scale);
+ result = DatumGetNumeric(DirectFunctionCall2(&numeric_mul, amount, numeric_scale));
+
+ result->n_sign_dscale = NUMERIC_SIGN(result) | fpoint; /* Display the right number of decimal digits. */
+
+ PG_RETURN_NUMERIC(result);
+ }
+
+ /* numeric_cash()
+ * Convert numeric to cash.
+ */
+ Datum
+ numeric_cash(PG_FUNCTION_ARGS)
+ {
+ Datum amount = PG_GETARG_DATUM(0);
+ Cash result;
+ int fpoint;
+ int64 scale;
+ int i;
+ Datum numeric_scale;
+
+ struct lconv *lconvert = PGLC_localeconv();
+
+ /*
+ * Find the number of digits after the decimal point.
+ */
+ fpoint = lconvert->frac_digits;
+ if (fpoint < 0 || fpoint > 10)
+ fpoint = 2;
+ scale = 1;
+ for (i = 0; i < fpoint; i++)
+ scale *= 10;
+
+ numeric_scale = DirectFunctionCall1(&int8_numeric, Int64GetDatum(scale));
+ amount = DirectFunctionCall2(&numeric_mul, amount, numeric_scale);
+ amount = DirectFunctionCall1(&numeric_int8, amount);
+
+ result = DatumGetInt64(amount);
+ PG_RETURN_CASH(result);
+ }
*** ./src/include/catalog/pg_cast.h.orig 2010-05-31 14:52:30.000000000 -0700
--- ./src/include/catalog/pg_cast.h 2010-05-31 14:54:02.000000000 -0700
***************
*** 124,129 ****
--- 124,131 ----
DATA(insert ( 1700 23 1744 a f ));
DATA(insert ( 1700 700 1745 i f ));
DATA(insert ( 1700 701 1746 i f ));
+ DATA(insert ( 790 1700 3823 a f ));
+ DATA(insert ( 1700 790 3824 a f ));
/* Allow explicit coercions between int4 and bool */
DATA(insert ( 23 16 2557 e f ));
*** ./src/include/catalog/pg_operator.h.orig 2010-05-31 14:52:30.000000000 -0700
--- ./src/include/catalog/pg_operator.h 2010-05-31 14:54:02.000000000 -0700
***************
*** 943,948 ****
--- 943,951 ----
DATA(insert OID = 2992 ( "<=" PGNSP PGUID b f f 2249 2249 16 2993 2991 record_le scalarltsel scalarltjoinsel ));
DATA(insert OID = 2993 ( ">=" PGNSP PGUID b f f 2249 2249 16 2992 2990 record_ge scalargtsel scalargtjoinsel ));
+ /* enhancement to money type */
+ DATA(insert OID = 3825 ( "/" PGNSP PGUID b f f 790 790 701 0 0 cash_div_cash - - ));
+
/*
* function prototypes
*** ./src/include/catalog/pg_proc.h.orig 2010-05-31 14:52:30.000000000 -0700
--- ./src/include/catalog/pg_proc.h 2010-05-31 14:54:02.000000000 -0700
***************
*** 4778,4783 ****
--- 4778,4791 ----
DATA(insert OID = 3114 ( nth_value PGNSP PGUID 12 1 0 0 f t f t f i 2 0 2283 "2283 23" _null_ _null_ _null_ _null_ window_nth_value _null_ _null_ _null_ ));
DESCR("fetch the Nth row value");
+ /* enhancements to money type */
+ DATA(insert OID = 3822 ( cash_div_cash PGNSP PGUID 12 1 0 0 f f f t f i 2 0 701 "790 790" _null_ _null_ _null_ _null_ cash_div_cash _null_ _null_ _null_ ));
+ DESCR("divide");
+ DATA(insert OID = 3823 ( numeric PGNSP PGUID 12 1 0 0 f f f t f i 1 0 1700 "790" _null_ _null_ _null_ _null_ cash_numeric _null_ _null_ _null_ ));
+ DESCR("(internal)");
+ DATA(insert OID = 3824 ( money PGNSP PGUID 12 1 0 0 f f f t f i 1 0 790 "1700" _null_ _null_ _null_ _null_ numeric_cash _null_ _null_ _null_ ));
+ DESCR("(internal)");
+
/*
* Symbolic values for provolatile column: these indicate whether the result
*** ./src/include/utils/cash.h.orig 2010-05-31 14:52:44.000000000 -0700
--- ./src/include/utils/cash.h 2010-05-31 14:54:02.000000000 -0700
***************
*** 63,66 ****
--- 63,70 ----
extern Datum cash_words(PG_FUNCTION_ARGS);
+ extern Datum cash_div_cash(PG_FUNCTION_ARGS);
+ extern Datum cash_numeric(PG_FUNCTION_ARGS);
+ extern Datum numeric_cash(PG_FUNCTION_ARGS);
+
#endif /* CASH_H */
Andy Balholm <andy@balholm.com> wrote:
On May 30, 2010, at 6:53 AM, Kevin Grittner wrote:
You would then generate a diff in context format and post to the
-hackers list with that file as an attachment.Here it is:
[context diff attachment]
I can't add it to the CommitFest page, since I don't have web
access, just e-mail. Could you please take care of that part?
Done. I'll keep it up-to-date as other posts occur.
(What is the CommitFest page, anyway?)
"A CommitFest is a periodic break to PostgreSQL development that
focuses on patch review and commit rather than new development."
These are held so that all the work for a release gets relatively
prompt review and feedback, and so that work for a release doesn't
pile up until the end of the release cycle. During these periods
developers are asked to review and test the patches submitted by
others. The hope is that this will also reduce the burden on those
who do the final review and commit. The CF page is used to keep
track of submitted patches and their status, to help manage the
process.
When we're not trying to put together a major release CFs tend to
run for one month each with a one month gap between them. Due to
the process of getting a release out the door, though, the last one
started on the 15th of January and the next one starts on the 15th
of July. We're going to try to get some early review on as many
patches as possible starting the 15th of June, but the committers
probably won't have much time to deal with any of this until the
official CF, so we're calling this (less formal) period a "Review
Fest".
The peer review process often results in discussion on the -hackers
list and/or requests for some sort of modification before commit.
Most patches wind up getting committed, although some are returned
with feedback (in hopes that the submitter will make some change
and submit to a later cycle) or rejected (if they are determined by
the community not to be useful changes).
By the way, I signed on to review your patch.
-Kevin
Thanks for the explanation of CommitFests.
On May 30, 2010, at 6:53 AM, Kevin Grittner wrote:
You would then generate a diff in context format and post to the
-hackers list with that file as an attachment.
I made my diff with src/tools/make_diff, as suggested in the Developer FAQ. But using git diff would be less hassle. Do the diffs from git diff work just as well?
Andy Balholm wrote:
Thanks for the explanation of CommitFests.
On May 30, 2010, at 6:53 AM, Kevin Grittner wrote:
You would then generate a diff in context format and post to the
-hackers list with that file as an attachment.I made my diff with src/tools/make_diff, as suggested in the
Developer FAQ. But using git diff would be less hassle. Do the
diffs from git diff work just as well?
Yes, of course.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ None of us is going to be here forever. +
Andy Balholm wrote:
Thanks for the explanation of CommitFests.
On May 30, 2010, at 6:53 AM, Kevin Grittner wrote:
You would then generate a diff in context format and post to the
-hackers list with that file as an attachment.I made my diff with src/tools/make_diff, as suggested in the Developer FAQ. But using git diff would be less hassle. Do the diffs from git diff work just as well?
context diffs are preferred - for advise on how to create them:
http://wiki.postgresql.org/wiki/Working_with_Git#Context_diffs_with_Git
Stefan
Andy Balholm <andy@balholm.com> wrote:
I made my diff with src/tools/make_diff, as suggested in the
Developer FAQ. But using git diff would be less hassle. Do the
diffs from git diff work just as well?
I agree about the git diff being easier; however, those files are in
unified format and some committers prefer to read the context
format, so I'd recommend piping it through filterdiff
--format=context. Personally, although I submit patches in context
format, I keep the unified copy around because I find the method
names from git useful and I like to be able to view the patch
through kompare, which doesn't seem to like the context format as
well.
-Kevin
Excerpts from Kevin Grittner's message of mar jun 01 11:09:38 -0400 2010:
I agree about the git diff being easier; however, those files are in
unified format and some committers prefer to read the context
format, so I'd recommend piping it through filterdiff
--format=context. Personally, although I submit patches in context
format, I keep the unified copy around because I find the method
names from git useful
Hmm, cvs diff -Ncp does show method names too, so this is probably
filterdiff removing them.
BTW maybe the developer faq could use all the info gathered in this
thread.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.com> wrote:
Hmm, cvs diff -Ncp does show method names too, so this is probably
filterdiff removing them.
My bad; I apparently got confused somehow when looking at a context
diff -- the function names are indeed there after the filterdiff
conversion. Sorry for the noise on that.
BTW maybe the developer faq could use all the info gathered in
this thread.
I'll take a look at that today.
-Kevin
[moving to -www list with bc to -hackers]
Alvaro Herrera <alvherre@commandprompt.com> wrote:
BTW maybe the developer faq could use all the info gathered in
this thread.
I wound up putting a few sentences from this thread into the
CommitFest Wiki page, and linking to that from the "Submitting a
Patch" and "Developer FAQ" pages. I think it might be good to also
have a link to the CommitFest Wiki page from the "CommitFest Index"
page in the application.
-Kevin
On Tue, Jun 1, 2010 at 3:17 PM, Kevin Grittner
<Kevin.Grittner@wicourts.gov> wrote:
[moving to -www list with bc to -hackers]
Alvaro Herrera <alvherre@commandprompt.com> wrote:
BTW maybe the developer faq could use all the info gathered in
this thread.I wound up putting a few sentences from this thread into the
CommitFest Wiki page, and linking to that from the "Submitting a
Patch" and "Developer FAQ" pages. I think it might be good to also
have a link to the CommitFest Wiki page from the "CommitFest Index"
page in the application.
Please feel free to send a patch.
http://git.postgresql.org/gitweb?p=pgcommitfest.git;a=summary
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company
Robert Haas <robertmhaas@gmail.com> wrote:
Kevin Grittner <Kevin.Grittner@wicourts.gov> wrote:
I think it might be good to also have a link to the CommitFest
Wiki page from the "CommitFest Index" page in the application.Please feel free to send a patch.
http://git.postgresql.org/gitweb?p=pgcommitfest.git;a=summary
Erm, I'm embarrassed to admit that I don't have a clue what a tt2 or
pm file *is* or what the style of web development used would be
called, but it kinda looks like adding some raw HTML near the top of
commitfest_view.tt2 would accomplish what I had in mind. If that
sounds good, I'll prepare the patch; if not, suggestions?
-Kevin
I wrote:
Robert Haas <robertmhaas@gmail.com> wrote:
Kevin Grittner <Kevin.Grittner@wicourts.gov> wrote:
I think it might be good to also have a link to the CommitFest
Wiki page from the "CommitFest Index" page in the application.Please feel free to send a patch.
it kinda looks like adding some raw HTML near the top of
commitfest_view.tt2 would accomplish what I had in mind.
I have no idea how to test this, but as a shot in the dark, see
attached.
-Kevin
Attachments:
pgcommitfest-wiki.patchtext/plain; name=pgcommitfest-wiki.patchDownload
diff --git a/template/commitfest_view.tt2 b/template/commitfest_view.tt2
index 5e4db85..9740307 100644
--- a/template/commitfest_view.tt2
+++ b/template/commitfest_view.tt2
@@ -1,6 +1,7 @@
<p>The most recent three comments for each patch will be displayed below. To
view all the comments for a particular patch, or to add a comment or make other
-changes, click on the patch name.</p>
+changes, click on the patch name.
+ (<a href='http://wiki.postgresql.org/CommitFest'>more info</a>)</p>
[% IF status_count_list.size > 0 %]
<p><b>Status Summary</b>.
I wrote:
I wrote:
Robert Haas <robertmhaas@gmail.com> wrote:
Kevin Grittner <Kevin.Grittner@wicourts.gov> wrote:
I think it might be good to also have a link to the CommitFest
Wiki page from the "CommitFest Index" page in the application.Please feel free to send a patch.
it kinda looks like adding some raw HTML near the top of
commitfest_view.tt2 would accomplish what I had in mind.I have no idea how to test this, but as a shot in the dark, see
attached.
Better try this, instead. (Bad URL in first try.)
-Kevin
Attachments:
pgcommitfest-wiki.patchtext/plain; name=pgcommitfest-wiki.patchDownload
diff --git a/template/commitfest_view.tt2 b/template/commitfest_view.tt2
index 5e4db85..4e43c88 100644
--- a/template/commitfest_view.tt2
+++ b/template/commitfest_view.tt2
@@ -1,6 +1,7 @@
<p>The most recent three comments for each patch will be displayed below. To
view all the comments for a particular patch, or to add a comment or make other
-changes, click on the patch name.</p>
+changes, click on the patch name.
+ (<a href='http://wiki.postgresql.org/wiki/CommitFest'>more info</a>)</p>
[% IF status_count_list.size > 0 %]
<p><b>Status Summary</b>.
Import Notes
Reply to msg id not found:
On Wed, Jun 2, 2010 at 6:03 PM, Kevin Grittner
<Kevin.Grittner@wicourts.gov> wrote:
I wrote:
I wrote:
Robert Haas <robertmhaas@gmail.com> wrote:
Kevin Grittner <Kevin.Grittner@wicourts.gov> wrote:
I think it might be good to also have a link to the CommitFest
Wiki page from the "CommitFest Index" page in the application.Please feel free to send a patch.
it kinda looks like adding some raw HTML near the top of
commitfest_view.tt2 would accomplish what I had in mind.I have no idea how to test this, but as a shot in the dark, see
attached.Better try this, instead. (Bad URL in first try.)
Hmm. I dunno if adding this to the commitfest view page is the best
place for it. Two other ideas that occur to me are (1) the home page
for the site, and (2) the page header (up there where it says "Home
Page" and "Log In").
Thoughts?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company
Andy Balholm <andy@balholm.com> wrote:
On May 30, 2010, at 6:53 AM, Kevin Grittner wrote:
You would then generate a diff in context format and post to the
-hackers list with that file as an attachment.Here it is
=================
Submission review
=================
* Is the patch in context diff format?
Yes, although the line endings are Windows format (CR/LF). The
patch utility on my system just ignored the CRs, but if they can be
filtered, all the better.
* Does it apply cleanly to the current CVS HEAD?
It does.
* Does it include reasonable tests, necessary doc patches, etc?
The doc patches seemed reasonable to me. There were no test
patches; I'm not sure if they're necessary.
================
Usability review
================
** Read what the patch is supposed to do, and consider:
* Does the patch actually implement that?
Yes.
* Do we want that?
I think we do -- it allows easy casting between money and numeric,
and allows one number to be divided by another to get a ratio.
* Do we already have it?
There are work-arounds, but they are clumsy and error-prone.
* Does it follow SQL spec, or the community-agreed behavior?
There was discussion on the lists, and this patch implements the
consensus, as far as I can determine.
* Does it include pg_dump support (if applicable)?
Not applicable.
* Are there dangers?
None that I can see.
* Have all the bases been covered?
The only possible issue is that cast from numeric to money lets
overflow be noticed and handled by the numeric_int8 function, which
puts out an error message on overflow which might be confusing
(ERROR: bigint out of range).
============
Feature test
============
** Apply the patch, compile it and test:
* Does the feature work as advertised?
Yes.
* Are there corner cases the author has failed to consider?
Just the content of the error message on the cast from numeric to
money (see above). I'm not sure whether it's worth addressing that
since the money class silently yields the wrong value everywhere
else. For example, if you cast the numeric to text and then cast it
to money, you'll quietly get the wrong amount rather than an error
-- the behavior of this patch on the cast from numeric seem like an
improvement compared to that; perhaps we should create a TODO entry
to include overflow checking with reasonable errors in *all* money
functions? Alternatively, we could modify this cast to behave the
same as the cast from text, but that hardly seems like an
improvement.
* Are there any assertion failures or crashes?
No.
==================
Performance review
==================
* Does the patch slow down simple tests?
No. It seems to provide a very slight performance improvement for
the tests run. For example, a loop through a million casts of a
money literal to text runs about 1% slower than a cast of the same
money literal to numeric and then to text; which is reasonable
because it avoids the need to insert commas and a dollar sign.
Given the number of tests, there's maybe a 10% chance that the
apparent slight improvement was just noise, but given the nature of
the patch, it seems reasonable to expect that there would be a
slight improvement.
* If it claims to improve performance, does it?
It makes no such claim.
* Does it slow down other things?
No.
=============
Coding review
=============
** Read the changes to the code in detail and consider:
* Does it follow the project coding guidelines?
The only issue is with the general guideline to make the new code
blend in with existing code:
http://wiki.postgresql.org/wiki/Submitting_a_Patch
| Generally, try to blend in with the surrounding code.
| Comments are for clarification not for delineating your code from
| the surroundings.
There are comments to set off the new code, and some of the new DATA
lines (and similar) are separated away from where one would expect
them to be if they had been included with the rest. Moving a few
lines and deleting a few comment lines would resolve it.
* Are there portability issues?
I don't think so.
* Will it work on Windows/BSD etc?
I think so.
* Are the comments sufficient and accurate?
They seem so to me.
* Does it do what it says, correctly?
It looks like it both in reading the code and in testing.
* Does it produce compiler warnings?
No.
* Can you make it crash?
No.
===================
Architecture review
===================
** Consider the changes to the code in the context of the project as
** a whole:
* Is everything done in a way that fits together coherently with
* other features/modules?
Yes.
* Are there interdependencies that can cause problems?
No.
=============
Review review
=============
** Did the reviewer cover all the things that kind of reviewer is
** supposed to do?
I think so.
I'm going to set this back to "Waiting on Author" for the minor
rearrangement suggested in "Coding review". I welcome any comments
from the community on the issue of the error message generated on
cast from numeric to money with an out-of-bounds value.
-Kevin
On Jun 21, 2010, at 11:47 AM, Kevin Grittner wrote:
Yes, although the line endings are Windows format (CR/LF).
The line endings must have gotten changed in transit. My original diff used just LF. I made it on a Mac.
The only issue is with the general guideline to make the new code
blend in with existing code:http://wiki.postgresql.org/wiki/Submitting_a_Patch
| Generally, try to blend in with the surrounding code.
| Comments are for clarification not for delineating your code from
| the surroundings.There are comments to set off the new code, and some of the new DATA
lines (and similar) are separated away from where one would expect
them to be if they had been included with the rest. Moving a few
lines and deleting a few comment lines would resolve it.
I deleted the excess comments and moved some lines around. Here it is with the changes.
Attachments:
dividing-money.diffapplication/octet-stream; name=dividing-money.diffDownload
*** a/doc/src/sgml/datatype.sgml
--- b/doc/src/sgml/datatype.sgml
***************
*** 843,863 **** ALTER SEQUENCE <replaceable class="parameter">tablename</replaceable>_<replaceab
floating-point literals, as well as typical
currency formatting, such as <literal>'$1,000.00'</literal>.
Output is generally in the latter form but depends on the locale.
! Non-quoted numeric values can be converted to <type>money</type> by
! casting the numeric value to <type>text</type> and then
! <type>money</type>, for example:
<programlisting>
! SELECT 1234::text::money;
</programlisting>
! There is no simple way of doing the reverse in a locale-independent
! manner, namely casting a <type>money</type> value to a numeric type.
! If you know the currency symbol and thousands separator you can use
! <function>regexp_replace()</>:
<programlisting>
! SELECT regexp_replace('52093.89'::money::text, '[$,]', '', 'g')::numeric;
</programlisting>
</para>
<para>
Since the output of this data type is locale-sensitive, it might not
--- 843,871 ----
floating-point literals, as well as typical
currency formatting, such as <literal>'$1,000.00'</literal>.
Output is generally in the latter form but depends on the locale.
! </para>
!
! <para>
! Values of the <type>numeric</type> data type can be cast to <type>money</type>.
! Other numeric types can be converted to <type>money</type> by casting to
! <type>numeric</type> first, for example:
<programlisting>
! SELECT 1234::numeric::money;
</programlisting>
! A <type>money</type> value can be cast to <type>numeric</type> without
! loss of precision. Conversion to other types could potentially lose precision,
! and it must be done in two stages:
<programlisting>
! SELECT '52093.89'::money::numeric::float;
</programlisting>
</para>
+
+ <para>
+ When a <type>money</type> value is divided by another <type>money</type> value,
+ the result is <type>double precision</type> (i.e. a pure number, not money);
+ the currency units cancel each other out in the division.
+ </para>
<para>
Since the output of this data type is locale-sensitive, it might not
*** a/src/backend/utils/adt/cash.c
--- b/src/backend/utils/adt/cash.c
***************
*** 27,32 ****
--- 27,33 ----
#include "utils/builtins.h"
#include "utils/cash.h"
#include "utils/pg_locale.h"
+ #include "utils/numeric.h"
#define CASH_BUFSZ 36
***************
*** 845,847 **** cash_words(PG_FUNCTION_ARGS)
--- 846,939 ----
/* return as text datum */
PG_RETURN_TEXT_P(cstring_to_text(buf));
}
+
+ /* cash_div_cash()
+ * Divide cash by cash, returning float8.
+ */
+ Datum
+ cash_div_cash(PG_FUNCTION_ARGS)
+ {
+ Cash dividend = PG_GETARG_CASH(0);
+ Cash divisor = PG_GETARG_CASH(1);
+ float8 quotient;
+
+ if (divisor == 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_DIVISION_BY_ZERO),
+ errmsg("division by zero")));
+
+ quotient = (float8)dividend / (float8)divisor;
+ PG_RETURN_FLOAT8(quotient);
+ }
+
+ /* cash_numeric()
+ * Convert cash to numeric.
+ */
+ Datum
+ cash_numeric(PG_FUNCTION_ARGS)
+ {
+ Cash money = PG_GETARG_CASH(0);
+ int fpoint;
+ int64 scale;
+ int i;
+ Numeric result;
+ Datum amount;
+ Datum numeric_scale;
+ Datum one;
+
+ struct lconv *lconvert = PGLC_localeconv();
+
+ /*
+ * Find the number of digits after the decimal point.
+ * (These lines were copied from cash_in().)
+ */
+ fpoint = lconvert->frac_digits;
+ if (fpoint < 0 || fpoint > 10)
+ fpoint = 2;
+ scale = 1;
+ for (i = 0; i < fpoint; i++)
+ scale *= 10;
+
+ amount = DirectFunctionCall1(&int8_numeric, Int64GetDatum(money));
+ one = DirectFunctionCall1(&int8_numeric, Int64GetDatum(1));
+ numeric_scale = DirectFunctionCall1(&int8_numeric, Int64GetDatum(scale));
+ numeric_scale = DirectFunctionCall2(&numeric_div, one, numeric_scale);
+ result = DatumGetNumeric(DirectFunctionCall2(&numeric_mul, amount, numeric_scale));
+
+ result->n_sign_dscale = NUMERIC_SIGN(result) | fpoint; /* Display the right number of decimal digits. */
+
+ PG_RETURN_NUMERIC(result);
+ }
+
+ /* numeric_cash()
+ * Convert numeric to cash.
+ */
+ Datum
+ numeric_cash(PG_FUNCTION_ARGS)
+ {
+ Datum amount = PG_GETARG_DATUM(0);
+ Cash result;
+ int fpoint;
+ int64 scale;
+ int i;
+ Datum numeric_scale;
+
+ struct lconv *lconvert = PGLC_localeconv();
+
+ /*
+ * Find the number of digits after the decimal point.
+ */
+ fpoint = lconvert->frac_digits;
+ if (fpoint < 0 || fpoint > 10)
+ fpoint = 2;
+ scale = 1;
+ for (i = 0; i < fpoint; i++)
+ scale *= 10;
+
+ numeric_scale = DirectFunctionCall1(&int8_numeric, Int64GetDatum(scale));
+ amount = DirectFunctionCall2(&numeric_mul, amount, numeric_scale);
+ amount = DirectFunctionCall1(&numeric_int8, amount);
+
+ result = DatumGetInt64(amount);
+ PG_RETURN_CASH(result);
+ }
*** a/src/include/catalog/pg_cast.h
--- b/src/include/catalog/pg_cast.h
***************
*** 124,129 **** DATA(insert ( 1700 21 1783 a f ));
--- 124,131 ----
DATA(insert ( 1700 23 1744 a f ));
DATA(insert ( 1700 700 1745 i f ));
DATA(insert ( 1700 701 1746 i f ));
+ DATA(insert ( 790 1700 3823 a f ));
+ DATA(insert ( 1700 790 3824 a f ));
/* Allow explicit coercions between int4 and bool */
DATA(insert ( 23 16 2557 e f ));
*** a/src/include/catalog/pg_operator.h
--- b/src/include/catalog/pg_operator.h
***************
*** 415,420 **** DATA(insert OID = 915 ( "/" PGNSP PGUID b f f 790 21 790 0 0 cash_div_
--- 415,421 ----
DATA(insert OID = 916 ( "*" PGNSP PGUID b f f 701 790 790 908 0 flt8_mul_cash - - ));
DATA(insert OID = 917 ( "*" PGNSP PGUID b f f 23 790 790 912 0 int4_mul_cash - - ));
DATA(insert OID = 918 ( "*" PGNSP PGUID b f f 21 790 790 914 0 int2_mul_cash - - ));
+ DATA(insert OID = 3825 ( "/" PGNSP PGUID b f f 790 790 701 0 0 cash_div_cash - - ));
DATA(insert OID = 965 ( "^" PGNSP PGUID b f f 701 701 701 0 0 dpow - - ));
DATA(insert OID = 966 ( "+" PGNSP PGUID b f f 1034 1033 1034 0 0 aclinsert - - ));
*** a/src/include/catalog/pg_proc.h
--- b/src/include/catalog/pg_proc.h
***************
*** 1195,1202 **** DATA(insert OID = 899 ( cashsmaller PGNSP PGUID 12 1 0 0 f f f t f i 2 0 79
DESCR("smaller of two");
DATA(insert OID = 919 ( flt8_mul_cash PGNSP PGUID 12 1 0 0 f f f t f i 2 0 790 "701 790" _null_ _null_ _null_ _null_ flt8_mul_cash _null_ _null_ _null_ ));
DESCR("multiply");
! DATA(insert OID = 935 ( cash_words PGNSP PGUID 12 1 0 0 f f f t f i 1 0 25 "790" _null_ _null_ _null_ _null_ cash_words _null_ _null_ _null_ ));
DESCR("output amount as words");
/* OIDS 900 - 999 */
--- 1195,1208 ----
DESCR("smaller of two");
DATA(insert OID = 919 ( flt8_mul_cash PGNSP PGUID 12 1 0 0 f f f t f i 2 0 790 "701 790" _null_ _null_ _null_ _null_ flt8_mul_cash _null_ _null_ _null_ ));
DESCR("multiply");
! DATA(insert OID = 935 ( cash_words PGNSP PGUID 12 1 0 0 f f f t f i 1 0 25 "790" _null_ _null_ _null_ _null_ cash_words _null_ _null_ _null_ ));
DESCR("output amount as words");
+ DATA(insert OID = 3822 ( cash_div_cash PGNSP PGUID 12 1 0 0 f f f t f i 2 0 701 "790 790" _null_ _null_ _null_ _null_ cash_div_cash _null_ _null_ _null_ ));
+ DESCR("divide");
+ DATA(insert OID = 3823 ( numeric PGNSP PGUID 12 1 0 0 f f f t f i 1 0 1700 "790" _null_ _null_ _null_ _null_ cash_numeric _null_ _null_ _null_ ));
+ DESCR("(internal)");
+ DATA(insert OID = 3824 ( money PGNSP PGUID 12 1 0 0 f f f t f i 1 0 790 "1700" _null_ _null_ _null_ _null_ numeric_cash _null_ _null_ _null_ ));
+ DESCR("(internal)");
/* OIDS 900 - 999 */
*** a/src/include/utils/cash.h
--- b/src/include/utils/cash.h
***************
*** 63,66 **** extern Datum cashsmaller(PG_FUNCTION_ARGS);
--- 63,70 ----
extern Datum cash_words(PG_FUNCTION_ARGS);
+ extern Datum cash_div_cash(PG_FUNCTION_ARGS);
+ extern Datum cash_numeric(PG_FUNCTION_ARGS);
+ extern Datum numeric_cash(PG_FUNCTION_ARGS);
+
#endif /* CASH_H */
Andy Balholm <andy@balholm.com> wrote:
On Jun 21, 2010, at 11:47 AM, Kevin Grittner wrote:
The only issue is with the general guideline to make the new code
blend in with existing code:
I deleted the excess comments and moved some lines around. Here it
is with the changes.
I ran pgindent to standardize the white space. (No changes of
substance.) Patch attached.
I'll mark it "Ready for Committer".
Thanks, Andy!
Note to committer: I'm not set up to generate docs, so I just
eyeballed the sgml.
-Kevin
Attachments:
dividing-money-2.diffapplication/octet-stream; name=dividing-money-2.diffDownload
*** a/doc/src/sgml/datatype.sgml
--- b/doc/src/sgml/datatype.sgml
***************
*** 843,865 **** ALTER SEQUENCE <replaceable class="parameter">tablename</replaceable>_<replaceab
floating-point literals, as well as typical
currency formatting, such as <literal>'$1,000.00'</literal>.
Output is generally in the latter form but depends on the locale.
! Non-quoted numeric values can be converted to <type>money</type> by
! casting the numeric value to <type>text</type> and then
! <type>money</type>, for example:
<programlisting>
! SELECT 1234::text::money;
</programlisting>
! There is no simple way of doing the reverse in a locale-independent
! manner, namely casting a <type>money</type> value to a numeric type.
! If you know the currency symbol and thousands separator you can use
! <function>regexp_replace()</>:
<programlisting>
! SELECT regexp_replace('52093.89'::money::text, '[$,]', '', 'g')::numeric;
</programlisting>
</para>
<para>
Since the output of this data type is locale-sensitive, it might not
work to load <type>money</> data into a database that has a different
setting of <varname>lc_monetary</>. To avoid problems, before
--- 843,873 ----
floating-point literals, as well as typical
currency formatting, such as <literal>'$1,000.00'</literal>.
Output is generally in the latter form but depends on the locale.
! </para>
!
! <para>
! Values of the <type>numeric</type> data type can be cast to <type>money</type>.
! Other numeric types can be converted to <type>money</type> by casting to
! <type>numeric</type> first, for example:
<programlisting>
! SELECT 1234::numeric::money;
</programlisting>
! A <type>money</type> value can be cast to <type>numeric</type> without
! loss of precision. Conversion to other types could potentially lose precision,
! and it must be done in two stages:
<programlisting>
! SELECT '52093.89'::money::numeric::float;
</programlisting>
</para>
<para>
+ When a <type>money</type> value is divided by another <type>money</type> value,
+ the result is <type>double precision</type> (i.e. a pure number, not money);
+ the currency units cancel each other out in the division.
+ </para>
+
+ <para>
Since the output of this data type is locale-sensitive, it might not
work to load <type>money</> data into a database that has a different
setting of <varname>lc_monetary</>. To avoid problems, before
*** a/src/backend/utils/adt/cash.c
--- b/src/backend/utils/adt/cash.c
***************
*** 27,32 ****
--- 27,33 ----
#include "utils/builtins.h"
#include "utils/cash.h"
#include "utils/pg_locale.h"
+ #include "utils/numeric.h"
#define CASH_BUFSZ 36
***************
*** 845,847 **** cash_words(PG_FUNCTION_ARGS)
--- 846,941 ----
/* return as text datum */
PG_RETURN_TEXT_P(cstring_to_text(buf));
}
+
+ /* cash_div_cash()
+ * Divide cash by cash, returning float8.
+ */
+ Datum
+ cash_div_cash(PG_FUNCTION_ARGS)
+ {
+ Cash dividend = PG_GETARG_CASH(0);
+ Cash divisor = PG_GETARG_CASH(1);
+ float8 quotient;
+
+ if (divisor == 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_DIVISION_BY_ZERO),
+ errmsg("division by zero")));
+
+ quotient = (float8) dividend / (float8) divisor;
+ PG_RETURN_FLOAT8(quotient);
+ }
+
+ /* cash_numeric()
+ * Convert cash to numeric.
+ */
+ Datum
+ cash_numeric(PG_FUNCTION_ARGS)
+ {
+ Cash money = PG_GETARG_CASH(0);
+ int fpoint;
+ int64 scale;
+ int i;
+ Numeric result;
+ Datum amount;
+ Datum numeric_scale;
+ Datum one;
+
+ struct lconv *lconvert = PGLC_localeconv();
+
+ /*
+ * Find the number of digits after the decimal point. (These lines were
+ * copied from cash_in().)
+ */
+ fpoint = lconvert->frac_digits;
+ if (fpoint < 0 || fpoint > 10)
+ fpoint = 2;
+ scale = 1;
+ for (i = 0; i < fpoint; i++)
+ scale *= 10;
+
+ amount = DirectFunctionCall1(&int8_numeric, Int64GetDatum(money));
+ one = DirectFunctionCall1(&int8_numeric, Int64GetDatum(1));
+ numeric_scale = DirectFunctionCall1(&int8_numeric, Int64GetDatum(scale));
+ numeric_scale = DirectFunctionCall2(&numeric_div, one, numeric_scale);
+ result = DatumGetNumeric(DirectFunctionCall2(&numeric_mul, amount, numeric_scale));
+
+ result->n_sign_dscale = NUMERIC_SIGN(result) | fpoint; /* Display the right
+ * number of decimal
+ * digits. */
+
+ PG_RETURN_NUMERIC(result);
+ }
+
+ /* numeric_cash()
+ * Convert numeric to cash.
+ */
+ Datum
+ numeric_cash(PG_FUNCTION_ARGS)
+ {
+ Datum amount = PG_GETARG_DATUM(0);
+ Cash result;
+ int fpoint;
+ int64 scale;
+ int i;
+ Datum numeric_scale;
+
+ struct lconv *lconvert = PGLC_localeconv();
+
+ /*
+ * Find the number of digits after the decimal point.
+ */
+ fpoint = lconvert->frac_digits;
+ if (fpoint < 0 || fpoint > 10)
+ fpoint = 2;
+ scale = 1;
+ for (i = 0; i < fpoint; i++)
+ scale *= 10;
+
+ numeric_scale = DirectFunctionCall1(&int8_numeric, Int64GetDatum(scale));
+ amount = DirectFunctionCall2(&numeric_mul, amount, numeric_scale);
+ amount = DirectFunctionCall1(&numeric_int8, amount);
+
+ result = DatumGetInt64(amount);
+ PG_RETURN_CASH(result);
+ }
*** a/src/include/catalog/pg_cast.h
--- b/src/include/catalog/pg_cast.h
***************
*** 124,129 **** DATA(insert ( 1700 21 1783 a f ));
--- 124,131 ----
DATA(insert ( 1700 23 1744 a f ));
DATA(insert ( 1700 700 1745 i f ));
DATA(insert ( 1700 701 1746 i f ));
+ DATA(insert ( 790 1700 3823 a f ));
+ DATA(insert ( 1700 790 3824 a f ));
/* Allow explicit coercions between int4 and bool */
DATA(insert ( 23 16 2557 e f ));
*** a/src/include/catalog/pg_operator.h
--- b/src/include/catalog/pg_operator.h
***************
*** 415,420 **** DATA(insert OID = 915 ( "/" PGNSP PGUID b f f 790 21 790 0 0 cash_div_
--- 415,421 ----
DATA(insert OID = 916 ( "*" PGNSP PGUID b f f 701 790 790 908 0 flt8_mul_cash - - ));
DATA(insert OID = 917 ( "*" PGNSP PGUID b f f 23 790 790 912 0 int4_mul_cash - - ));
DATA(insert OID = 918 ( "*" PGNSP PGUID b f f 21 790 790 914 0 int2_mul_cash - - ));
+ DATA(insert OID = 3825 ( "/" PGNSP PGUID b f f 790 790 701 0 0 cash_div_cash - - ));
DATA(insert OID = 965 ( "^" PGNSP PGUID b f f 701 701 701 0 0 dpow - - ));
DATA(insert OID = 966 ( "+" PGNSP PGUID b f f 1034 1033 1034 0 0 aclinsert - - ));
*** a/src/include/catalog/pg_proc.h
--- b/src/include/catalog/pg_proc.h
***************
*** 1197,1202 **** DATA(insert OID = 919 ( flt8_mul_cash PGNSP PGUID 12 1 0 0 f f f t f i 2 0
--- 1197,1208 ----
DESCR("multiply");
DATA(insert OID = 935 ( cash_words PGNSP PGUID 12 1 0 0 f f f t f i 1 0 25 "790" _null_ _null_ _null_ _null_ cash_words _null_ _null_ _null_ ));
DESCR("output amount as words");
+ DATA(insert OID = 3822 ( cash_div_cash PGNSP PGUID 12 1 0 0 f f f t f i 2 0 701 "790 790" _null_ _null_ _null_ _null_ cash_div_cash _null_ _null_ _null_ ));
+ DESCR("divide");
+ DATA(insert OID = 3823 ( numeric PGNSP PGUID 12 1 0 0 f f f t f i 1 0 1700 "790" _null_ _null_ _null_ _null_ cash_numeric _null_ _null_ _null_ ));
+ DESCR("(internal)");
+ DATA(insert OID = 3824 ( money PGNSP PGUID 12 1 0 0 f f f t f i 1 0 790 "1700" _null_ _null_ _null_ _null_ numeric_cash _null_ _null_ _null_ ));
+ DESCR("(internal)");
/* OIDS 900 - 999 */
*** a/src/include/utils/cash.h
--- b/src/include/utils/cash.h
***************
*** 63,66 **** extern Datum cashsmaller(PG_FUNCTION_ARGS);
--- 63,70 ----
extern Datum cash_words(PG_FUNCTION_ARGS);
+ extern Datum cash_div_cash(PG_FUNCTION_ARGS);
+ extern Datum cash_numeric(PG_FUNCTION_ARGS);
+ extern Datum numeric_cash(PG_FUNCTION_ARGS);
+
#endif /* CASH_H */
"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
Andy Balholm <andy@balholm.com> wrote:
On Jun 21, 2010, at 11:47 AM, Kevin Grittner wrote:
I deleted the excess comments and moved some lines around. Here it
is with the changes.
I ran pgindent to standardize the white space. (No changes of
substance.) Patch attached.
I'll mark it "Ready for Committer".
Applied with some editorial changes. The noncosmetic changes were:
* I didn't like this bit in cash_numeric():
result->n_sign_dscale = NUMERIC_SIGN(result) | fpoint;
Not only is that unwarranted chumminess with the implementation of
numeric, it's flat-out wrong. If the result isn't exactly the right
number of digits (say, it's 12.33999999 instead of the desired 12.34)
this just hides the extra digits, it doesn't make the result correct.
The right way is to use numeric_round(), which not only sets the dscale
where we want it but rounds off any inaccuracy that might have crept in
from the division.
* The cast functions were marked immutable, which is wrong because they
depend on the setting of lc_monetary. The right marking is "stable".
It struck me in connection with the latter that cash_in and cash_out
were also improperly marked as immutable --- they also depend on
lc_monetary and so can be no better than stable. I changed that
in this commit. I'm not sure if it's worth trying to back-patch;
in practice people probably aren't changing lc_monetary on the fly,
and the volatility of I/O functions is usually not that interesting
anyway.
regards, tom lane
On tor, 2010-07-15 at 22:25 -0400, Tom Lane wrote:
"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
Andy Balholm <andy@balholm.com> wrote:
On Jun 21, 2010, at 11:47 AM, Kevin Grittner wrote:
I deleted the excess comments and moved some lines around. Here it
is with the changes.I ran pgindent to standardize the white space. (No changes of
substance.) Patch attached.I'll mark it "Ready for Committer".
Applied with some editorial changes.
I didn't see any discussion about why this should return float8 rather
than numeric. It seems wrong to use float8 for this.
Tom Lane <tgl@sss.pgh.pa.us> wrote:
* I didn't like this bit in cash_numeric():
result->n_sign_dscale = NUMERIC_SIGN(result) | fpoint;
Not only is that unwarranted chumminess with the implementation of
numeric, it's flat-out wrong. If the result isn't exactly the
right number of digits (say, it's 12.33999999 instead of the
desired 12.34) this just hides the extra digits, it doesn't make
the result correct. The right way is to use numeric_round(),
which not only sets the dscale where we want it but rounds off any
inaccuracy that might have crept in from the division.
Thanks. Duly noted.
* The cast functions were marked immutable, which is wrong because
they depend on the setting of lc_monetary. The right marking is
"stable".
Is there any impact of the change to lc_monetary which would matter
besides the number of decimal positions? If that changes, isn't
every money amount in the database instantly made incorrect? If so,
I'm dubious that marking this as stable is worthwhile -- if someone
is making a change like that, they will need to update all money
amounts in the database; reindexing would be the least of their
problems. Or am I missing some other effect?
-Kevin
Peter Eisentraut <peter_e@gmx.net> wrote:
I didn't see any discussion about why this should return float8
rather than numeric. It seems wrong to use float8 for this.
That discussion took place several months ago on the -bugs list.
I'll paste some links from a quick search of the archives below.
Since multiplication of money is by float8 and not numeric, it
ultimately seemed more consistent to me to have the results of
division be float8. I felt that as long as we had a cast between
money and numeric, someone could always cast to numeric if they
wanted that style of division.
http://archives.postgresql.org/pgsql-bugs/2010-03/msg00233.php
http://archives.postgresql.org/pgsql-bugs/2010-03/msg00241.php
http://archives.postgresql.org/pgsql-bugs/2010-03/msg00244.php
http://archives.postgresql.org/pgsql-bugs/2010-03/msg00245.php
http://archives.postgresql.org/pgsql-bugs/2010-04/msg00006.php
-Kevin
On Jul 15, 2010, at 7:25 PM, Tom Lane wrote:
* I didn't like this bit in cash_numeric():
result->n_sign_dscale = NUMERIC_SIGN(result) | fpoint;
Not only is that unwarranted chumminess with the implementation of
numeric, it's flat-out wrong. If the result isn't exactly the right
number of digits (say, it's 12.33999999 instead of the desired 12.34)
this just hides the extra digits, it doesn't make the result correct.
The right way is to use numeric_round(), which not only sets the dscale
where we want it but rounds off any inaccuracy that might have crept in
from the division.
Sorry about that. Is there documentation anywhere for backend functions and types? I couldn't find any, so I just looked through numeric.h to see what looked like it might work. I didn't find numeric_round, since it's declared in builtins.h.
"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
Peter Eisentraut <peter_e@gmx.net> wrote:
I didn't see any discussion about why this should return float8
rather than numeric. It seems wrong to use float8 for this.
That discussion took place several months ago on the -bugs list.
I'll paste some links from a quick search of the archives below.
Since multiplication of money is by float8 and not numeric, it
ultimately seemed more consistent to me to have the results of
division be float8. I felt that as long as we had a cast between
money and numeric, someone could always cast to numeric if they
wanted that style of division.
Yeah. The other argument that I found convincing was that if the
operator was defined to yield numeric, people might think that
the result was exact ... which of course it won't be, either way.
Choosing float8 helps to remind the user it's an approximate quotient.
regards, tom lane
Andy Balholm <andy@balholm.com> writes:
On Jul 15, 2010, at 7:25 PM, Tom Lane wrote:
* I didn't like this bit in cash_numeric():
result->n_sign_dscale = NUMERIC_SIGN(result) | fpoint;
Not only is that unwarranted chumminess with the implementation of
numeric, it's flat-out wrong. If the result isn't exactly the right
number of digits (say, it's 12.33999999 instead of the desired 12.34)
this just hides the extra digits, it doesn't make the result correct.
The right way is to use numeric_round(), which not only sets the dscale
where we want it but rounds off any inaccuracy that might have crept in
from the division.
Sorry about that. Is there documentation anywhere for backend
functions and types?
Nothing at that level of detail, unfortunately, beyond the code itself.
If you'd read the comments near the head of numeric.c, maybe the mistake
would've been apparent to you, or maybe not.
regards, tom lane
"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
Tom Lane <tgl@sss.pgh.pa.us> wrote:
* The cast functions were marked immutable, which is wrong because
they depend on the setting of lc_monetary. The right marking is
"stable".
Is there any impact of the change to lc_monetary which would matter
besides the number of decimal positions? If that changes, isn't
every money amount in the database instantly made incorrect?
Yeah, which is why I didn't feel that this was something that really
needed back-patching, even though the markings have been wrong since
the lc_monetary dependency was introduced.
If so,
I'm dubious that marking this as stable is worthwhile -- if someone
is making a change like that, they will need to update all money
amounts in the database; reindexing would be the least of their
problems. Or am I missing some other effect?
Well, whether people change the value in practice or not, it's still
wrong to mark the functions more optimistically than the rules say.
The only way I'd be willing to label those things immutable was if we
did something to lock down lc_monetary for the life of a database (ie,
make it work more like lc_collate does now). Which might be a good
idea, but it's not how it works today.
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> wrote:
The only way I'd be willing to label those things immutable was if
we did something to lock down lc_monetary for the life of a
database (ie, make it work more like lc_collate does now). Which
might be a good idea, but it's not how it works today.
Interesting. In general, what is involved in locking something like
this down for the life of a database?
-Kevin
"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
Tom Lane <tgl@sss.pgh.pa.us> wrote:
The only way I'd be willing to label those things immutable was if
we did something to lock down lc_monetary for the life of a
database (ie, make it work more like lc_collate does now). Which
might be a good idea, but it's not how it works today.
Interesting. In general, what is involved in locking something like
this down for the life of a database?
IIRC, the main pain point is providing an option for CREATE DATABASE
to set the value. If you chase down all the references to lc_collate
you'll get the picture.
It'd probably be worth doing if money were less deprecated, but right
now I can't get excited about it.
Actually ... the thing that might turn money into a less deprecated type
is if you could set lc_monetary per column. I wonder whether Peter's
collation hack could be extended to deal with that.
regards, tom lane
On fre, 2010-07-16 at 12:21 -0400, Tom Lane wrote:
Actually ... the thing that might turn money into a less deprecated
type
is if you could set lc_monetary per column. I wonder whether Peter's
collation hack could be extended to deal with that.
In principle yes.
On fre, 2010-07-16 at 08:55 -0500, Kevin Grittner wrote:
Peter Eisentraut <peter_e@gmx.net> wrote:
I didn't see any discussion about why this should return float8
rather than numeric. It seems wrong to use float8 for this.That discussion took place several months ago on the -bugs list.
I'll paste some links from a quick search of the archives below.
Since multiplication of money is by float8 and not numeric, it
ultimately seemed more consistent to me to have the results of
division be float8. I felt that as long as we had a cast between
money and numeric, someone could always cast to numeric if they
wanted that style of division.http://archives.postgresql.org/pgsql-bugs/2010-03/msg00233.php
http://archives.postgresql.org/pgsql-bugs/2010-03/msg00241.php
http://archives.postgresql.org/pgsql-bugs/2010-03/msg00244.php
http://archives.postgresql.org/pgsql-bugs/2010-03/msg00245.php
http://archives.postgresql.org/pgsql-bugs/2010-04/msg00006.php
I read most of these messages rather as advocating the use of NUMERIC.
Also, the multiplication problem can be addressed by adding a money *
numeric operator.
On fre, 2010-07-16 at 10:31 -0400, Tom Lane wrote:
The other argument that I found convincing was that if the
operator was defined to yield numeric, people might think that
the result was exact ... which of course it won't be, either way.
Choosing float8 helps to remind the user it's an approximate quotient.
Why is it approximate? Aren't money values really integers?
On Jul 17, 2010, at 3:20 AM, Peter Eisentraut wrote:
On fre, 2010-07-16 at 10:31 -0400, Tom Lane wrote:
The other argument that I found convincing was that if the
operator was defined to yield numeric, people might think that
the result was exact ... which of course it won't be, either way.
Choosing float8 helps to remind the user it's an approximate quotient.Why is it approximate? Aren't money values really integers?
$1.00 / 3.00 = 0.333333333333333333333333333333333333333333333333...
Peter Eisentraut <peter_e@gmx.net> writes:
On fre, 2010-07-16 at 10:31 -0400, Tom Lane wrote:
The other argument that I found convincing was that if the
operator was defined to yield numeric, people might think that
the result was exact ... which of course it won't be, either way.
Choosing float8 helps to remind the user it's an approximate quotient.
Why is it approximate? Aren't money values really integers?
1/3 is going to yield an approximate result either way.
regards, tom lane
Peter Eisentraut <peter_e@gmx.net> wrote:
I read most of these messages rather as advocating the use of
NUMERIC.
Yeah, I did advocate that at first, but became convinced float8 was
more appropriate.
Also, the multiplication problem can be addressed by adding a
money * numeric operator.
True. If we added money * numeric, then it would make more sense to
have money / money return numeric. On the other hand, I couldn't
come up with enough use cases for that to feel that it justified the
performance hit on money / money for typical use cases -- you
normally want a ratio for things where float8 is more than
sufficient; and you can always cast the arguments to numeric for
calculations where the approximate result isn't good enough.
Basically, once we agreed to include casts to and from numeric, it
seemed to me we had it covered.
We're certainly in much better shape to handle exact calculations
now that we have the casts than we were before.
-Kevin
On lör, 2010-07-17 at 07:20 -0700, Andy Balholm wrote:
On Jul 17, 2010, at 3:20 AM, Peter Eisentraut wrote:
On fre, 2010-07-16 at 10:31 -0400, Tom Lane wrote:
The other argument that I found convincing was that if the
operator was defined to yield numeric, people might think that
the result was exact ... which of course it won't be, either way.
Choosing float8 helps to remind the user it's an approximate quotient.Why is it approximate? Aren't money values really integers?
$1.00 / 3.00 = 0.333333333333333333333333333333333333333333333333...
By that reasoning, numeric / numeric should also yield float.
On lör, 2010-07-17 at 10:00 -0500, Kevin Grittner wrote:
True. If we added money * numeric, then it would make more sense to
have money / money return numeric. On the other hand, I couldn't
come up with enough use cases for that to feel that it justified the
performance hit on money / money for typical use cases -- you
normally want a ratio for things where float8 is more than
sufficient; and you can always cast the arguments to numeric for
calculations where the approximate result isn't good enough.
Basically, once we agreed to include casts to and from numeric, it
seemed to me we had it covered.
I have never used the money type, so I'm not in a position to argue what
might be typical use cases, but it is well understood that using
floating-point arithmetic anywhere in calculations involving money is
prohibited by law or business rules in most places. So when I read that
multiplications or divisions involving the money type use float, to me
that means the same as "never use the money type, it's broken".
Peter Eisentraut <peter_e@gmx.net> writes:
I have never used the money type, so I'm not in a position to argue what
might be typical use cases, but it is well understood that using
floating-point arithmetic anywhere in calculations involving money is
prohibited by law or business rules in most places. So when I read that
multiplications or divisions involving the money type use float, to me
that means the same as "never use the money type, it's broken".
[ shrug... ] A lot of people think that about the money type, all for
different reasons. This particular argument seems tissue-thin to me,
mainly because the same people who complain "it must be exact" have no
problem rounding off their results to the nearest pfennig or whatever.
Also, you seem not to have absorbed the fact that changing the output
to numeric *will not make the result exact anyway*. If the point of
a business rule of this sort is to prohibit inexact calculations, then
having it flag cash / cash as inexact is a Good Thing.
regards, tom lane