Re: [HACKERS] dividing money by money
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.... By that reasoning, numeric / numeric should also yield float. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dividing money by money
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dividing money by money
Peter Eisentraut pete...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dividing money by money
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dividing money by money
On fre, 2010-07-16 at 08:55 -0500, Kevin Grittner wrote: Peter Eisentraut pete...@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/msg6.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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dividing money by money
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? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dividing money by money
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.... -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dividing money by money
Peter Eisentraut pete...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dividing money by money
Peter Eisentraut pete...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dividing money by money
Tom Lane t...@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.3399 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dividing money by money
Peter Eisentraut pete...@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/msg6.php -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dividing money by money
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.3399 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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dividing money by money
Kevin Grittner kevin.gritt...@wicourts.gov writes: Peter Eisentraut pete...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dividing money by money
Andy Balholm a...@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.3399 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dividing money by money
Kevin Grittner kevin.gritt...@wicourts.gov writes: Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dividing money by money
Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dividing money by money
Kevin Grittner kevin.gritt...@wicourts.gov writes: Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dividing money by money
Kevin Grittner kevin.gritt...@wicourts.gov writes: Andy Balholm a...@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.3399 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dividing money by money
On tor, 2010-07-15 at 22:25 -0400, Tom Lane wrote: Kevin Grittner kevin.gritt...@wicourts.gov writes: Andy Balholm a...@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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dividing money by money
Andy Balholm a...@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 dividing-money-2.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dividing money by money
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
Re: [HACKERS] dividing money by money
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. dividing-money.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dividing money by money
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: [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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dividing money by money
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? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dividing money by money
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 br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + None of us is going to be here forever. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dividing money by money
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dividing money by money
Andy Balholm a...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dividing money by money
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 alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dividing money by money
Alvaro Herrera alvhe...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dividing money by money
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: dividing-money.diff Description: Binary data Don't forget to add it to the CommitFest page: https://commitfest.postgresql.org/action/commitfest_view/open 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? (What is the CommitFest page, anyway?) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dividing money by money
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? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dividing money by money
Andy Balholm a...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers