Fwd: Re: [BUGS] dividing money by money

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

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:

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

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

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