Обсуждение: 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)
...
Cash newvalue = (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)
{
Cash c1 = PG_GETARG_CASH(0);
Cash c2 = PG_GETARG_CASH(1);
Cash result;
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
On Mon, Dec 11, 2017 at 5:50 PM, Andres Freund <andres@anarazel.de> 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+=n2ynkR1kdTHECnoruWS_G@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
Robert Haas <robertmhaas@gmail.com> 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
Hi, On 2017-12-12 11:01:04 -0500, Tom Lane wrote: > Robert Haas <robertmhaas@gmail.com> 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
Andres Freund <andres@anarazel.de> writes:
>> Robert Haas <robertmhaas@gmail.com> 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
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
On Wed, Dec 13, 2017 at 6:50 AM, Andres Freund <andres@anarazel.de> 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
On 2017-12-13 08:27:42 +0900, Michael Paquier wrote: > On Wed, Dec 13, 2017 at 6:50 AM, Andres Freund <andres@anarazel.de> 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
On Wed, Dec 13, 2017 at 8:30 AM, Andres Freund <andres@anarazel.de> 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