Re: money type's overflow handling is woefully incomplete
On Wed, Dec 13, 2017 at 8:30 AM, Andres Freund wrote: > On 2017-12-13 08:27:42 +0900, Michael Paquier wrote: >> >> (OTOH, I bet we could drop reltime/abstime without too many complaints. >> >> Y2038 is coming.) >> > >> > I'm actually about to send a patch doing so, that code is one mess WRT >> > overflow handling. >> >> Agreed. I think as well that those should be fixed. It does not seem >> much complicated to fix them. > > I'm not following. I was trying to say that I'll send a patch removing > the abstime/reltime/tinterval code. My mistake here. I thought that you were specifically referring to the money data type here. I did not parse your previous message correctly. -- Michael
Re: money type's overflow handling is woefully incomplete
On 2017-12-13 08:27:42 +0900, Michael Paquier wrote: > On Wed, Dec 13, 2017 at 6:50 AM, Andres Freund wrote: > > On 2017-12-12 16:47:17 -0500, Tom Lane wrote: > >> Really? We've got test cases that intentionally exercise overflow > >> in the money code? I think we could just drop such tests, until > >> such time as someone fixes the issue. > > > > Some parts at least (input & output), I think it's easy enough to fix > > those up. > > There could be two ways to fix that: > 1) Call the int8 equivalents with DirectFunctionCall2 and rely on the > overflow there, but this has a performance impact. > 2) Add similar checks as in int8.c, which feels like duplicating code > but those are cheap. > You are heading to 2) I guess? I don't think 1) makes much sense. The error messages will be bad, and the harder cases (e.g. cash_in()) can't share code anyway. > >> (OTOH, I bet we could drop reltime/abstime without too many complaints. > >> Y2038 is coming.) > > > > I'm actually about to send a patch doing so, that code is one mess WRT > > overflow handling. > > Agreed. I think as well that those should be fixed. It does not seem > much complicated to fix them. I'm not following. I was trying to say that I'll send a patch removing the abstime/reltime/tinterval code. Greetings, Andres Freund
Re: money type's overflow handling is woefully incomplete
On Wed, Dec 13, 2017 at 6:50 AM, Andres Freund wrote: > On 2017-12-12 16:47:17 -0500, Tom Lane wrote: >> Really? We've got test cases that intentionally exercise overflow >> in the money code? I think we could just drop such tests, until >> such time as someone fixes the issue. > > Some parts at least (input & output), I think it's easy enough to fix > those up. There could be two ways to fix that: 1) Call the int8 equivalents with DirectFunctionCall2 and rely on the overflow there, but this has a performance impact. 2) Add similar checks as in int8.c, which feels like duplicating code but those are cheap. You are heading to 2) I guess? >> (OTOH, I bet we could drop reltime/abstime without too many complaints. >> Y2038 is coming.) > > I'm actually about to send a patch doing so, that code is one mess WRT > overflow handling. Agreed. I think as well that those should be fixed. It does not seem much complicated to fix them. -- Michael
Re: money type's overflow handling is woefully incomplete
On 2017-12-12 16:47:17 -0500, Tom Lane wrote: > Really? We've got test cases that intentionally exercise overflow > in the money code? I think we could just drop such tests, until > such time as someone fixes the issue. Some parts at least (input & output), I think it's easy enough to fix those up. > (OTOH, I bet we could drop reltime/abstime without too many complaints. > Y2038 is coming.) I'm actually about to send a patch doing so, that code is one mess WRT overflow handling. Greetings, Andres Freund
Re: money type's overflow handling is woefully incomplete
Andres Freund writes: >> Robert Haas writes: >>> Long story short, I don't think anyone cares about this enough to >>> spend effort fixing it. I suspect the money data type has very few >>> users. > I'm unfortunately not so sure :(. There seem to be enough of them that we get pushback anytime somebody suggests removing the type. > The background is that I was working on committing the faster (& > correct) overflow checks, and wanted to compile postgres with -ftrapv. > Some rudimentary cash (as well as pgbench, rel/abstime) fixes were > required to make the tests succeed... Really? We've got test cases that intentionally exercise overflow in the money code? I think we could just drop such tests, until such time as someone fixes the issue. (OTOH, I bet we could drop reltime/abstime without too many complaints. Y2038 is coming.) regards, tom lane
Re: money type's overflow handling is woefully incomplete
Hi, On 2017-12-12 11:01:04 -0500, Tom Lane wrote: > Robert Haas writes: > > Long story short, I don't think anyone cares about this enough to > > spend effort fixing it. I suspect the money data type has very few > > users. I'm unfortunately not so sure :(. > Somebody did contribute the effort not too long ago to install that > overflow check into cash_in. So maybe somebody will step up and fix > the other money functions. I can't get too excited about it in the > meantime. I can't really either. But I think that kinda suggest we ought to rip that code out in the not too far away future. The background is that I was working on committing the faster (& correct) overflow checks, and wanted to compile postgres with -ftrapv. Some rudimentary cash (as well as pgbench, rel/abstime) fixes were required to make the tests succeed... Greetings, Andres Freund
Re: money type's overflow handling is woefully incomplete
Robert Haas writes: > Long story short, I don't think anyone cares about this enough to > spend effort fixing it. I suspect the money data type has very few > users. Somebody did contribute the effort not too long ago to install that overflow check into cash_in. So maybe somebody will step up and fix the other money functions. I can't get too excited about it in the meantime. regards, tom lane
Re: money type's overflow handling is woefully incomplete
On Mon, Dec 11, 2017 at 5:50 PM, Andres Freund wrote: > This leads to fun like: > > postgres[2002][1]=# SELECT '92233720368547758.07'::money+'0.1'; > ┌─┐ > │ ?column? │ > ├─┤ > │ -$92,233,720,368,547,757.99 │ > └─┘ It seems like I could have a lot MORE fun if I did it the other way -- i.e. spent myself so deeply into debt that I went positive again. Seriously, though, this same issue was noted in a discussion back in 2011: https://www.postgresql.org/message-id/flat/AANLkTi%3Dzbyy2%3Dcq8Wa3K3%2B%3Dn2ynkR1kdTHECnoruWS_G%40mail.gmail.com#AANLkTi=zbyy2=cq8Wa3K3+=n2ynkr1kdthecnoruw...@mail.gmail.com Long story short, I don't think anyone cares about this enough to spend effort fixing it. I suspect the money data type has very few users. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
money type's overflow handling is woefully incomplete
Hi, The money type has overflow handling in its input function: Datum cash_in(PG_FUNCTION_ARGS) ... Cashnewvalue = (value * 10) - (*s - '0'); if (newvalue / 10 != value) ereport(ERROR, (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), errmsg("value \"%s\" is out of range for type %s", str, "money"))); but not in a lot of other relevant places: Datum cash_pl(PG_FUNCTION_ARGS) { Cashc1 = PG_GETARG_CASH(0); Cashc2 = PG_GETARG_CASH(1); Cashresult; result = c1 + c2; PG_RETURN_CASH(result); } Same with cash_mi, int8_mul_cash, cash_div_int8, etc. cash_out doesn't have a plain overflow, but: if (value < 0) { /* make the amount positive for digit-reconstruction loop */ value = -value; doesn't reliably work if value is PG_INT64_MIN. This leads to fun like: postgres[2002][1]=# SELECT '92233720368547758.07'::money+'0.1'; ┌─┐ │ ?column? │ ├─┤ │ -$92,233,720,368,547,757.99 │ └─┘ Greetings, Andres Freund