Re: plpgsql FOR loop doesn't guard against strange step values

I just noticed that when the BY option was added to plpgsql FOR loops,
no real error checking was done. If you specify a zero step value,
you'll have an infinite loop. If you specify a negative value, the
loop variable will increment in the "wrong direction" until integer
overflow occurs. Neither of these behaviors seem desirable in the
least.

Another problem is that no check for overflow is done when incrementing
the loop variable, which means that an infinite loop is possible if the
step value is larger than the distance from the loop upper bound to
INT_MAX --- the loop variable could overflow before it is seen to be
greater than the upper bound, and after wrapping around to negative
it's still less than the upper bound, so the loop continues to run.

I suggest throwing an error for zero or negative step value, and
terminating the loop if the loop variable overflows.

Any objections?

regards, tom lane

Tom Lane wrote:
> I just noticed that when the BY option was added to plpgsql FOR
> loops, no real error checking was done.  If you specify a zero step
> value, you'll have an infinite loop.  If you specify a negative
> value, the loop variable will increment in the "wrong direction"
> until integer overflow occurs.  Neither of these behaviors seem
> desirable in the least.

That seems to be fairly normal proramming language behavior.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> Tom Lane wrote:
>> I just noticed that when the BY option was added to plpgsql FOR
>> loops, no real error checking was done. If you specify a zero step
>> value, you'll have an infinite loop. If you specify a negative
>> value, the loop variable will increment in the "wrong direction"
>> until integer overflow occurs. Neither of these behaviors seem
>> desirable in the least.

> That seems to be fairly normal proramming language behavior.

Well, it's about what I'd expect from C or something at a similar level
of (non) abstraction. But I dislike the idea that plpgsql should have
behavior as machine-dependent as that the number of iterations will
depend on the value of INT_MIN. Also, at the SQL level our usual policy
is to throw errors for obvious programmer mistakes, and it's hard to
argue that a zero or negative step isn't a programmer mistake. Had we
defined the stepping behavior differently (ie, make "BY -1" work like
REVERSE) then there would be some sanity in allowing negative steps,
but I don't see the sanity in it given the implemented behavior.

regards, tom lane

Tom Lane wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
>
>> Tom Lane wrote:
>>
>>> I just noticed that when the BY option was added to plpgsql FOR
>>> loops, no real error checking was done. If you specify a zero step
>>> value, you'll have an infinite loop. If you specify a negative
>>> value, the loop variable will increment in the "wrong direction"
>>> until integer overflow occurs. Neither of these behaviors seem
>>> desirable in the least.
>>>
>
>
>> That seems to be fairly normal proramming language behavior.
>>
>
> Well, it's about what I'd expect from C or something at a similar level
> of (non) abstraction. But I dislike the idea that plpgsql should have
> behavior as machine-dependent as that the number of iterations will
> depend on the value of INT_MIN. Also, at the SQL level our usual policy
> is to throw errors for obvious programmer mistakes, and it's hard to
> argue that a zero or negative step isn't a programmer mistake. Had we
> defined the stepping behavior differently (ie, make "BY -1" work like
> REVERSE) then there would be some sanity in allowing negative steps,
> but I don't see the sanity in it given the implemented behavior.
>

I suspect we have a significant incompatibility with PLSQL in this area.
The docs give this example:

FOR i IN REVERSE 10..1 LOOP
-- some computations here
END LOOP;

In PLSQL, as I understand it, (and certainly in its ancestor Ada) this loop will execute 0 times, not 10. To iterate from 10 down to 1 one would need to say:

FOR i IN REVERSE 1..10 LOOP
-- some computations here
END LOOP;

I'm not sure if this has been noticed before. It's actually quite unfortunate. At least it should be mentioned in the section of the docs relating to porting from PLSQL.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> I suspect we have a significant incompatibility with PLSQL in this area.

Ugh. Google seems to confirm your thought that Oracle expects

> FOR i IN REVERSE 1..10 LOOP

which is not the way we are doing it. Not sure if it's worth trying to
fix this --- the conversion pain would be significant. I agree we gotta
document it, however; will go do so.

Note that in the Oracle worldview it still wouldn't be sensible to use
a negative step.

regards, tom lane

Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>
>> I suspect we have a significant incompatibility with PLSQL in this area.
>>
>
> Ugh. Google seems to confirm your thought that Oracle expects
>
>
>> FOR i IN REVERSE 1..10 LOOP
>>
>
> which is not the way we are doing it. Not sure if it's worth trying to
> fix this --- the conversion pain would be significant. I agree we gotta
> document it, however; will go do so.
>
> Note that in the Oracle worldview it still wouldn't be sensible to use
> a negative step.
>
>
>

Quite so. I think we should probably require the step to be greater than
0, whether or not we are using REVERSE, and choose to use it as an
increment or decrement as appropriate.

cheers

andrew

On 7/14/07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I just noticed that when the BY option was added to plpgsql FOR loops,
> no real error checking was done. If you specify a zero step value,
> you'll have an infinite loop. If you specify a negative value, the
> loop variable will increment in the "wrong direction" until integer
> overflow occurs. Neither of these behaviors seem desirable in the
> least.
>

while i read this the day you posted it, i didn't have time to answer
until now...

my answer is: sorry, my bad... have to admit that the idea of
preventing zero in the BY doesn't cross my mind.

http://archives.postgresql.org/pgsql-hackers/2006-04/msg01100.php
i remember my original proposal include that negative values shouldn't
be allowed, i don't know where my way was corrupted... maybe because
for statement didn't make any effort to prevent things like:
FOR i IN 10..1 LOOP

> Another problem is that no check for overflow is done when incrementing
> the loop variable, which means that an infinite loop is possible if the
> step value is larger than the distance from the loop upper bound to
> INT_MAX --- the loop variable could overflow before it is seen to be
> greater than the upper bound, and after wrapping around to negative
> it's still less than the upper bound, so the loop continues to run.
>

mmm... yeah!

> I suggest throwing an error for zero or negative step value, and
> terminating the loop if the loop variable overflows.
>

http://archives.postgresql.org/pgsql-committers/2007-07/msg00142.php
at least the part that prevents overflow and probably the one that
reject zero in BY are clearly bugs and should be backpatched to 8.2,
aren't they?

--
regards,
Jaime Casanova

"Programming today is a race between software engineers striving to
build bigger and better idiot-proof programs and the universe trying
to produce bigger and better idiots.
So far, the universe is winning."
Richard Cook

"Jaime Casanova" <systemguards(at)gmail(dot)com> writes:
> http://archives.postgresql.org/pgsql-committers/2007-07/msg00142.php
> at least the part that prevents overflow and probably the one that
> reject zero in BY are clearly bugs and should be backpatched to 8.2,
> aren't they?

Well, it's a behavioral change, so given the lack of complaints from the
field I'm inclined not to back-patch.

regards, tom lane