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:
/messages/by-id/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