Thread: pgsql: Use new overflow aware integer operations.
Use new overflow aware integer operations. A previous commit added inline functions that provide fast(er) and correct overflow checks for signed integer math. Use them in a significant portion of backend code. There's more to touch in both backend and frontend code, but these were the easily identifiable cases. The old overflow checks are noticeable in integer heavy workloads. A secondary benefit is that getting rid of overflow checks that rely on signed integer overflow wrapping around, will allow us to get rid of -fwrapv in the future. Which in turn slows down other code. Author: Andres Freund Discussion: https://postgr.es/m/20171024103954.ztmatprlglz3rwke@alap3.anarazel.de Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/101c7ee3ee847bac970c74b73b4f2858484383e5 Modified Files -------------- contrib/btree_gist/btree_cash.c | 10 +- contrib/btree_gist/btree_int2.c | 10 +- contrib/btree_gist/btree_int4.c | 10 +- contrib/btree_gist/btree_int8.c | 10 +- contrib/btree_gist/btree_utils_num.h | 2 - src/backend/utils/adt/array_userfuncs.c | 13 +- src/backend/utils/adt/cash.c | 39 ++-- src/backend/utils/adt/float.c | 9 +- src/backend/utils/adt/int.c | 200 ++++------------- src/backend/utils/adt/int8.c | 377 ++++++++------------------------ src/backend/utils/adt/numeric.c | 41 ++-- src/backend/utils/adt/oracle_compat.c | 18 +- src/backend/utils/adt/varbit.c | 4 +- src/backend/utils/adt/varlena.c | 13 +- 14 files changed, 217 insertions(+), 539 deletions(-)
On 13 December 2017 at 14:01, Andres Freund <andres@anarazel.de> wrote: > Use new overflow aware integer operations. Thanks for making this happen. I notice it's caused a small warning in compilers that don't understand about elog(ERROR) and ereport(ERROR) not returning. This can be seen on bowerbird's compile log [1]: c:\prog\bf\root\head\pgsql.build\src\backend\utils\adt\int8.c(131): warning C4715: 'scanint8' : not all control paths return a value [c:\prog\bf\root\HEAD\pgsql.build\postgres.vcxproj] The attached just shuffles things around to get rid of the warning. This way seems better than to add another "return false" at the end as this way saves a couple of lines of code rather than adding one. [1] https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=bowerbird&dt=2017-12-15%2018%3A21%3A51&stg=make -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Attachment
David Rowley <david.rowley@2ndquadrant.com> writes:
> I notice it's caused a small warning in compilers that don't
> understand about elog(ERROR) and ereport(ERROR) not returning.
Wups, I noticed this independently and fixed it before reading your
message. Sorry about failing to credit you in the commit log.
regards, tom lane
Andres Freund <andres@anarazel.de> writes:
> Use new overflow aware integer operations.
I just tried to compile manually on prairiedog for the first time since
this went in, and noticed that it now produces a boatload of warnings
like
int.c: In function 'int2pl':
int.c:735: warning: 'result' may be used uninitialized in this function
I think it's entirely within its rights to complain, because
the non-builtin code paths in int.h look like
int32 res = (int32) a + (int32) b;
if (res > PG_INT16_MAX || res < PG_INT16_MIN)
return true;
*result = (int16) res;
return false;
and so if an overflow occurs then *result won't get set.
I do not think it is reasonable for these functions to not set the
output variable at all in the overflow case; it is not their job
to opine on whether the caller may use the result. Furthermore,
this is an incorrect emulation of the built-in functions;
the gcc manual clearly says
These built-in functions promote the first two operands into
infinite precision signed type and perform addition on those
promoted operands. The result is then cast to the type the third
pointer argument points to and stored there. If the stored result
is equal to the infinite precision result, the built-in functions
return false, otherwise they return true.
So as coded, there is a difference of behavior depending on whether
you have the built-in, and that cannot be anything but bad.
Therefore I think all of these need a patch like
int32 res = (int32) a + (int32) b;
+ *result = (int16) res;
if (res > PG_INT16_MAX || res < PG_INT16_MIN)
return true;
- *result = (int16) res;
return false;
regards, tom lane
Hi, On 2017-12-22 11:00:54 -0500, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > Use new overflow aware integer operations. > > I just tried to compile manually on prairiedog for the first time since > this went in, and noticed that it now produces a boatload of warnings > like > > int.c: In function 'int2pl': > int.c:735: warning: 'result' may be used uninitialized in this function > > I think it's entirely within its rights to complain, because > the non-builtin code paths in int.h look like > > int32 res = (int32) a + (int32) b; > > if (res > PG_INT16_MAX || res < PG_INT16_MIN) > return true; > *result = (int16) res; > return false; > > and so if an overflow occurs then *result won't get set. > > I do not think it is reasonable for these functions to not set the > output variable at all in the overflow case; it is not their job > to opine on whether the caller may use the result. I don't agree. Please note that that the function's documentation explicitly says "The content of *result is implementation defined in case of overflow.". > Furthermore, this is an incorrect emulation of the built-in functions; > the gcc manual clearly says > These built-in functions promote the first two operands into > infinite precision signed type and perform addition on those > promoted operands. The result is then cast to the type the third > pointer argument points to and stored there. If the stored result > is equal to the infinite precision result, the built-in functions > return false, otherwise they return true. > > So as coded, there is a difference of behavior depending on whether > you have the built-in, and that cannot be anything but bad. The problem is that other intrinsic implementation we might want to use in the future, like microsoft's, do *not* exactly behave that way. So I think defining this as implementation defined is the right thing to do. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> On 2017-12-22 11:00:54 -0500, Tom Lane wrote:
>> I do not think it is reasonable for these functions to not set the
>> output variable at all in the overflow case; it is not their job
>> to opine on whether the caller may use the result.
> I don't agree. Please note that that the function's documentation
> explicitly says "The content of *result is implementation defined in
> case of overflow.".
I will not accept an implementation that spews compiler warnings
all over the place, which is what this one is doing. Please fix that,
or else I will.
regards, tom lane
On December 22, 2017 7:52:54 PM GMT+01:00, Tom Lane <tgl@sss.pgh.pa.us> wrote: >Andres Freund <andres@anarazel.de> writes: >> On 2017-12-22 11:00:54 -0500, Tom Lane wrote: >>> I do not think it is reasonable for these functions to not set the >>> output variable at all in the overflow case; it is not their job >>> to opine on whether the caller may use the result. > >> I don't agree. Please note that that the function's documentation >> explicitly says "The content of *result is implementation defined in >> case of overflow.". > >I will not accept an implementation that spews compiler warnings >all over the place, which is what this one is doing. Please fix that, >or else I will. Are you seriously implying that I'm suggesting that we live with a warning / that I refuse to fix one? All I was saying isthat I don't want to exactly define which value *result is set to in case of overflow. Without having resolved the discussionof semantics it just seemed pointless to start fixing... Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
[ back from Christmas break ]
Andres Freund <andres@anarazel.de> writes:
> On December 22, 2017 7:52:54 PM GMT+01:00, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I will not accept an implementation that spews compiler warnings
>> all over the place, which is what this one is doing. Please fix that,
>> or else I will.
> Are you seriously implying that I'm suggesting that we live with a warning / that I refuse to fix one? All I was
sayingis that I don't want to exactly define which value *result is set to in case of overflow. Without having resolved
thediscussion of semantics it just seemed pointless to start fixing...
Sorry, I was being unnecessarily grumpy there. I follow your point about
not wanting to constrain the implementation to yield the correct low-order
bits if we don't actually need that behavior ... but I'm still not happy
about the warnings.
What do you think of having the fallback code explicitly set the output
variable to zero (or any other fixed value) on overflow, like
#if defined(HAVE__BUILTIN_OP_OVERFLOW)
return __builtin_add_overflow(a, b, result);
#else
int32 res = (int32) a + (int32) b;
if (res > PG_INT16_MAX || res < PG_INT16_MIN)
+ {
+ *result = 0; /* just to keep compiler quiet */
return true;
+ }
*result = (int16) res;
return false;
#endif
I do not think this would cause any performance loss in our expected
usage, because reasonably bright compilers would detect that the store
is dead code and remove it. But less-bright compilers would not be
issuing warnings.
regards, tom lane
On Wed, Dec 13, 2017 at 2:01 PM, Andres Freund <andres@anarazel.de> wrote: > Use new overflow aware integer operations. frogmouth seems to have crashed in the vicinity of various number processing tests -- could this commit be responsible? A wild guess as there isn't much to go on in the build log: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=frogmouth&dt=2017-12-28%2011%3A30%3A54 -- Thomas Munro http://www.enterprisedb.com
On 2017-12-29 23:06:10 +1300, Thomas Munro wrote: > On Wed, Dec 13, 2017 at 2:01 PM, Andres Freund <andres@anarazel.de> wrote: > > Use new overflow aware integer operations. > > frogmouth seems to have crashed in the vicinity of various number > processing tests -- could this commit be responsible? A wild guess as > there isn't much to go on in the build log: That seems not that likely an explanation, given there were a good number of runs without failure after this commit went in, before the animal started to fail due to PHJ. Greetings, Andres Freund
On 2017-12-27 17:59:26 -0500, Tom Lane wrote:
> [ back from Christmas break ]
>
> Andres Freund <andres@anarazel.de> writes:
> > On December 22, 2017 7:52:54 PM GMT+01:00, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> I will not accept an implementation that spews compiler warnings
> >> all over the place, which is what this one is doing. Please fix that,
> >> or else I will.
>
> > Are you seriously implying that I'm suggesting that we live with a warning / that I refuse to fix one? All I was
sayingis that I don't want to exactly define which value *result is set to in case of overflow. Without having resolved
thediscussion of semantics it just seemed pointless to start fixing...
>
> Sorry, I was being unnecessarily grumpy there.
Lack of holidays does that to one ;)
> I follow your point about not wanting to constrain the implementation
> to yield the correct low-order bits if we don't actually need that
> behavior ... but I'm still not happy about the warnings.
Agreed.
> What do you think of having the fallback code explicitly set the output
> variable to zero (or any other fixed value) on overflow, like
>
> #if defined(HAVE__BUILTIN_OP_OVERFLOW)
> return __builtin_add_overflow(a, b, result);
> #else
> int32 res = (int32) a + (int32) b;
>
> if (res > PG_INT16_MAX || res < PG_INT16_MIN)
> + {
> + *result = 0; /* just to keep compiler quiet */
> return true;
> + }
> *result = (int16) res;
> return false;
> #endif
>
> I do not think this would cause any performance loss in our expected
> usage, because reasonably bright compilers would detect that the store
> is dead code and remove it. But less-bright compilers would not be
> issuing warnings.
Yea, that works for me. I wonder if we should choose an absurd sentinel
value to prevent code from relying on one? 0x0000beef or such. Unless
somebody protests soon-ish I'll make it so.
Greetings,
Andres Freund
On Sat, Dec 30, 2017 at 8:43 AM, Andres Freund <andres@anarazel.de> wrote:
> On 2017-12-29 23:06:10 +1300, Thomas Munro wrote:
>> On Wed, Dec 13, 2017 at 2:01 PM, Andres Freund <andres@anarazel.de> wrote:
>> > Use new overflow aware integer operations.
>>
>> frogmouth seems to have crashed in the vicinity of various number
>> processing tests -- could this commit be responsible? A wild guess as
>> there isn't much to go on in the build log:
>
> That seems not that likely an explanation, given there were a good
> number of runs without failure after this commit went in, before the
> animal started to fail due to PHJ.
Hmm, yeah, also the more recent failure is bombing in different
places, so I take that back. I was thrown by a couple of bits in the
earlier build log that seemed to evoke this commit:
INSERT INTO FLOAT4_TBL(f1) VALUES ('10e70');
! ERROR: value out of range: overflow
SELECT * FROM generate_series('+4567890123456789'::int8,
'+4567890123456799'::int8, 0);
! WARNING: terminating connection because of crash of another server process
--
Thomas Munro
http://www.enterprisedb.com
On 2017-12-29 12:21:54 -0800, Andres Freund wrote:
> On 2017-12-27 17:59:26 -0500, Tom Lane wrote:
> > #if defined(HAVE__BUILTIN_OP_OVERFLOW)
> > return __builtin_add_overflow(a, b, result);
> > #else
> > int32 res = (int32) a + (int32) b;
> >
> > if (res > PG_INT16_MAX || res < PG_INT16_MIN)
> > + {
> > + *result = 0; /* just to keep compiler quiet */
> > return true;
> > + }
> > *result = (int16) res;
> > return false;
> > #endif
> >
> > I do not think this would cause any performance loss in our expected
> > usage, because reasonably bright compilers would detect that the store
> > is dead code and remove it. But less-bright compilers would not be
> > issuing warnings.
>
> Yea, that works for me. I wonder if we should choose an absurd sentinel
> value to prevent code from relying on one? 0x0000beef or such. Unless
> somebody protests soon-ish I'll make it so.
Pushed that way (with 0x5EED as the value, seems more appropriate ;)).
I can't convince any of my compilers to actual emit warnings in this
case, so we'll have to see whether prairiedog like this...
Greetings,
Andres Freund
Andres Freund <andres@anarazel.de> writes:
>> Yea, that works for me. I wonder if we should choose an absurd sentinel
>> value to prevent code from relying on one? 0x0000beef or such. Unless
>> somebody protests soon-ish I'll make it so.
> Pushed that way (with 0x5EED as the value, seems more appropriate ;)).
LGTM, thanks!
> I can't convince any of my compilers to actual emit warnings in this
> case, so we'll have to see whether prairiedog like this...
Per my results yesterday, locust and coypu were also complaining;
one of them might come back quicker.
regards, tom lane
Hi, On 2018-02-14 17:36:38 -0500, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > I can't convince any of my compilers to actual emit warnings in this > > case, so we'll have to see whether prairiedog like this... > > Per my results yesterday, locust and coypu were also complaining; > one of them might come back quicker. All of them seem to be ok with this now. Greetings, Andres Freund