Andy Balholm <a...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
