pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedmoney type's overflow handling is woefully incomplete
9+ messages / 4 participants
[nested] [flat]
* money type's overflow handling is woefully incomplete
@ 2017-12-11 22:50 Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 9+ messages in thread
From: Andres Freund @ 2017-12-11 22:50 UTC (permalink / raw)
To: pgsql-hackers
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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: money type's overflow handling is woefully incomplete
@ 2017-12-12 15:49 Robert Haas <robertmhaas@gmail.com>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 9+ messages in thread
From: Robert Haas @ 2017-12-12 15:49 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: pgsql-hackers
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%40m...
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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: money type's overflow handling is woefully incomplete
@ 2017-12-12 16:01 Tom Lane <tgl@sss.pgh.pa.us>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 9+ messages in thread
From: Tom Lane @ 2017-12-12 16:01 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Andres Freund <andres@anarazel.de>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: money type's overflow handling is woefully incomplete
@ 2017-12-12 18:21 Andres Freund <andres@anarazel.de>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 9+ messages in thread
From: Andres Freund @ 2017-12-12 18:21 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: money type's overflow handling is woefully incomplete
@ 2017-12-12 21:47 Tom Lane <tgl@sss.pgh.pa.us>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 9+ messages in thread
From: Tom Lane @ 2017-12-12 21:47 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: money type's overflow handling is woefully incomplete
@ 2017-12-12 21:50 Andres Freund <andres@anarazel.de>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 9+ messages in thread
From: Andres Freund @ 2017-12-12 21:50 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: money type's overflow handling is woefully incomplete
@ 2017-12-12 23:27 Michael Paquier <michael.paquier@gmail.com>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 9+ messages in thread
From: Michael Paquier @ 2017-12-12 23:27 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: money type's overflow handling is woefully incomplete
@ 2017-12-12 23:30 Andres Freund <andres@anarazel.de>
parent: Michael Paquier <michael.paquier@gmail.com>
0 siblings, 1 reply; 9+ messages in thread
From: Andres Freund @ 2017-12-12 23:30 UTC (permalink / raw)
To: Michael Paquier <michael.paquier@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: money type's overflow handling is woefully incomplete
@ 2017-12-13 00:04 Michael Paquier <michael.paquier@gmail.com>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 0 replies; 9+ messages in thread
From: Michael Paquier @ 2017-12-13 00:04 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 9+ messages in thread
end of thread, other threads:[~2017-12-13 00:04 UTC | newest]
Thread overview: 9+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2017-12-11 22:50 money type's overflow handling is woefully incomplete Andres Freund <andres@anarazel.de>
2017-12-12 15:49 ` Robert Haas <robertmhaas@gmail.com>
2017-12-12 16:01 ` Tom Lane <tgl@sss.pgh.pa.us>
2017-12-12 18:21 ` Andres Freund <andres@anarazel.de>
2017-12-12 21:47 ` Tom Lane <tgl@sss.pgh.pa.us>
2017-12-12 21:50 ` Andres Freund <andres@anarazel.de>
2017-12-12 23:27 ` Michael Paquier <michael.paquier@gmail.com>
2017-12-12 23:30 ` Andres Freund <andres@anarazel.de>
2017-12-13 00:04 ` Michael Paquier <michael.paquier@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox