On 03/17/2018 06:27 PM, Andres Freund wrote:
>
>
> On March 17, 2018 7:56:40 AM PDT, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I wrote:
>>> Ouch. That test is in fact new as of 31 Dec, and what this seems to
>>> prove is that plpgsql's handling of loop-variable overflow doesn't
>>> work on fulmar.
>>
>> Some of the other icc-using critters haven't reported in since
>> December, either :-(
>>
>> Looking at the code, we do this like so:
>>
>> /*
>> * Increase/decrease loop value, unless it would overflow, in which
>> * case exit the loop.
>> */
>> if (stmt->reverse)
>> {
>> if ((int32) (loop_value - step_value) > loop_value)
>> break;
>> loop_value -= step_value;
>> }
>> else
>> {
>> if ((int32) (loop_value + step_value) < loop_value)
>> break;
>> loop_value += step_value;
>> }
>>
>> I imagine what's happening is that the compiler is assuming no overflow
>> occurs (due to lacking any equivalent of -fwrapv), then deducing that
>> the
>> if-tests are no-ops and throwing them away.
>>
>> We could avoid the dependency on -fwrapv with something like
>>
>> if (stmt->reverse)
>> {
>> if (loop_value < (PG_INT32_MIN + step_value))
>> break;
>> loop_value -= step_value;
>> }
>> else
>> {
>> if (loop_value > (PG_INT32_MAX - step_value))
>> break;
>> loop_value += step_value;
>> }
>>
>> which is safe because we enforce step_value > 0. It's kind of ugly
>> because it hard-codes knowledge of what the limits are, but we may not
>> have much choice.
>
> On the current branch just using the new overflow safe functions in
> int.h should work. But unless we are OK leaving this broken in the back
> branches, or want to backport the functionality, that's probably not
> sufficient.
>
Not sure, but the backbranches seem to be working fine, and the commit
that triggers the issue is from December 31. Maybe the issue was there
but we were lucky not to trip on it before.
Anyway, I can confirm that the fix suggested by Tom does the trick
(well, at least on Fulmar, which is running icc 14.0.3). I've also
disassembled exec_stmt_fori both with and without the patch - reading
assembly in not my strength, but if you're interested it's attached. The
interesting part seems to be the last ~50 lines or so.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
