Re: dividing money by money

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

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(at)balholm(dot)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:

Andy Balholm <andy(at)balholm(dot)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(at)momjian(dot)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(at)balholm(dot)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(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)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(at)commandprompt(dot)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(dot)Grittner(at)wicourts(dot)gov> wrote:
> [moving to -www list with bc to -hackers]
>
> Alvaro Herrera <alvherre(at)commandprompt(dot)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(at)gmail(dot)com> wrote:
> Kevin Grittner <Kevin(dot)Grittner(at)wicourts(dot)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(at)gmail(dot)com> wrote:
>> Kevin Grittner <Kevin(dot)Grittner(at)wicourts(dot)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

I wrote:
> I wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> Kevin Grittner <Kevin(dot)Grittner(at)wicourts(dot)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

On Wed, Jun 2, 2010 at 6:03 PM, Kevin Grittner
<Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
> I wrote:
>> I wrote:
>>> Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>>> Kevin Grittner <Kevin(dot)Grittner(at)wicourts(dot)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(at)balholm(dot)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.

Andy Balholm <andy(at)balholm(dot)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

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
> Andy Balholm <andy(at)balholm(dot)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(dot)Grittner(at)wicourts(dot)gov> writes:
> > Andy Balholm <andy(at)balholm(dot)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(at)sss(dot)pgh(dot)pa(dot)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(at)gmx(dot)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(dot)Grittner(at)wicourts(dot)gov> writes:
> Peter Eisentraut <peter_e(at)gmx(dot)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(at)balholm(dot)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(dot)Grittner(at)wicourts(dot)gov> writes:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)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(dot)Grittner(at)wicourts(dot)gov> writes:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)gmx(dot)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(at)gmx(dot)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(at)gmx(dot)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(at)gmx(dot)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