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