On 03/17/2018 07:20 PM, Tom Lane wrote:
> Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
>> 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.
>
> Yeah, we were simply not testing that overflow-detection code before.
> Undoubtedly it would fail in the back branches too if we tested it.
>
>> 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.
>
> Hm, did you get the "master" and "patched" versions backwards? The
> allegedly-patched version does the !reverse case like this:
>
> 0x00007f71219457ae <+2200>: mov -0x108(%rbp),%eax
> 0x00007f71219457b4 <+2206>: test %eax,%eax
> 0x00007f71219457b6 <+2208>: jl 0x7f71219457cf <exec_stmt_fori+2233>
> 0x00007f71219457b8 <+2210>: mov -0x108(%rbp),%eax
> 0x00007f71219457be <+2216>: add -0x110(%rbp),%eax
> 0x00007f71219457c4 <+2222>: mov %eax,-0x110(%rbp)
> 0x00007f71219457ca <+2228>: jmpq 0x7f7121945573 <exec_stmt_fori+1629>
>
> so that it's apparently optimized
>
> if ((int32) (loop_value + step_value) < loop_value)
> break;
>
> into
>
> if (step_value < 0)
> break;
>
> which of course never exits the loop. That's slightly different
> (and stupider) than what I'd been hypothesizing, but it's a valid
> transformation if you ignore the possibility of integer overflow.
>
Yeah, it seems I've mixed up the files by accident. Sorry.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
