Re: BUG #8448: looping through query results exits at 10th step under some conditions

The following bug has been logged on the website:

Bug reference: 8448
Logged by: László Rózsahegyi
Email address: laszlo(dot)rozsahegyi(at)rool(dot)hu
PostgreSQL version: 9.3.0
Operating system: windows 7 64 bit
Description:

Looping through query results exits at 10th step when
* query has for update clause, and
* in loop body between 1 and 10 step update - at least one step - the locked
record

I tested it after a fresh and clean PostgreSQL install. The configuration
files left unchanged.

test code (bur_report.sql):
set client_encoding='UTF-8';

/* looping through query results exits at 10th step under some conditions
*/

create database looptest;

\c looptest

create sequence id_seq start 100;

create table test (
id bigint not null unique default nextval('id_seq')
, code varchar(3) not null
, note text
);

insert into test (code) values ('HUN'), ('ENG');

create type t_10 as (
num integer
, note text
);

select n.id, g.i
from test n
, generate_series(1,15) g(i)
where
n.code = 'HUN'
order by
2
;
/* The results are 15 rows */

create or replace function update10()
returns setof t_10
language plpgsql
volatile security definer
as
$BODY$
declare
lRec record;
lSor t_10;
begin
for lRec in
select n.id, g.i
from test n
, generate_series(1,15) g(i) /* 15 > 10 */
where
n.code = 'HUN'
order by
2
for update of n /* bug part 1 */
loop
lSor.num = lRec.i;
lSor.note = lRec.id::text || '-' || lRec.i::text;
/* exit loop after 10th step when update locked record in 1..10 step
otherwise returns all 15 record (example condition is lRec.i > 10 ) */
if lRec.i = 1 then
update test set note = lSor.note where id = lRec.id; /* bug part 2 */
end if;
return next lSor;
end loop;
return;
end;
$BODY$
;

select * from update10();
/* The results are 10 records */

\c postgres

drop database looptest;
-- end code

The last query results are 10 records. I expected 15 records, like the other
query.

I tested the code in windows 7 command prompt:

C:\temp>"c:\Program Files\PostgreSQL\9.3\bin\psql.exe" -Upostgres
-h127.0.0.1 -p5557 -f bug_report.sql
Password for user postgres:
SET
CREATE DATABASE
You are now connected to database "looptest" as user "postgres".
CREATE SEQUENCE
CREATE TABLE
INSERT 0 2
CREATE TYPE
id | i
-----+----
100 | 1
100 | 2
100 | 3
100 | 4
100 | 5
100 | 6
100 | 7
100 | 8
100 | 9
100 | 10
100 | 11
100 | 12
100 | 13
100 | 14
100 | 15
(15 rows)

CREATE FUNCTION
num | note
-----+--------
1 | 100-1
2 | 100-2
3 | 100-3
4 | 100-4
5 | 100-5
6 | 100-6
7 | 100-7
8 | 100-8
9 | 100-9
10 | 100-10
(10 rows)

You are now connected to database "postgres" as user "postgres".
DROP DATABASE

C:\temp>

laszlo(dot)rozsahegyi(at)rool(dot)hu writes:
> Looping through query results exits at 10th step when
> * query has for update clause, and
> * in loop body between 1 and 10 step update - at least one step - the locked
> record

I looked into this a bit. The key point is that the function has
a FOR loop over a SELECT FOR UPDATE, wherein the SELECT FOR UPDATE
would return the same row of table "test" multiple times (because
it's cross-joined to a generate_series() call). But the body of
the FOR loop updates the just-returned row of "test".

Now what should happen, according to the definition of SELECT FOR
UPDATE, is that once a later command in the same transaction has
updated a given row, SELECT FOR UPDATE shouldn't return that row
anymore. (See the HeapTupleSelfUpdated case in nodeLockRows.c.)
So what ought to happen here is that after the UPDATE, the FOR
loop finds no more records and falls out. (This may not be what
the submitter expected, but it's what should happen.)

However ... plpgsql FOR-over-query loops like to prefetch rows.
In this case, the FOR prefetches ten rows at startup and then
runs the loop body. It will then proceed to iterate over the
next nine rows, even though those would not have been returned
anymore by the underlying SELECT FOR UPDATE.

So basically, the bug here is that the prefetching behavior is
user-visible in a confusing way.

This doesn't seem like it's specific to SELECT FOR UPDATE, either.
In any situation where the FOR query has visible side effects,
the prefetching behavior is going to be user-visible if the loop
body does something that's sensitive to those side effects. This
would at least include INSERT/UPDATE/DELETE RETURNING and queries
containing volatile functions.

There are several things I can think of that we might do about this:

1. Leave the code alone, but document that the prefetching behavior exists.
Users who run into this are on their own to work around it.

2. Try to detect whether the FOR query has any visible side-effects,
and shut off prefetching if so.

3. Document the prefetching behavior and provide a way for users to
shut it off if it's a problem in their specific case.

I'm not particularly attracted by #2; it would be difficult to do with
100% confidence, and it would probably result in a noticeable performance
penalty, with benefits to only a very small percentage of users.
(The prefetch code has been there since 2001, and the number of complaints
about it would certainly not take more than one hand to count.)

#3 is probably the thing to do, but I'm not volunteering to do it
myself. Or maybe we should just do #1 --- again, the number of
people hitting this has been vanishingly small.

regards, tom lane

Tom Lane-2 wrote
> 1. Leave the code alone, but document that the prefetching behavior
> exists.
> Users who run into this are on their own to work around it.

1a. Leave the code alone and expect that occasionally users will encounter
this problem, post to the bugs/general list, and the community helps them
figure out how to overcome the problem in their specific case.

Even though the OPs test case proves the bug exists the fact that it makes
no sense indeed makes it hard to get excited about proactively preventing
this situation. Convention for updating within a loop is that you'd update
the current record on each iteration - not past or future records (which is
happening implicitly here because of the CROSS JOIN). A proper update
statement, possibly with a RETURNING clause, is often going to be the better
solution anyway.

So, is there an underlying use-case driving this complaint that should be
considered by the community either to induce a fix to the behavior or just
to solicit suggestions for better alternatives?

David J.

--
View this message in context: http://postgresql.1045698.n5.nabble.com/BUG-8448-looping-through-query-results-exits-at-10th-step-under-some-conditions-tp5770594p5777999.html
Sent from the PostgreSQL - bugs mailing list archive at Nabble.com.

David Johnston <polobo(at)yahoo(dot)com> writes:
> So, is there an underlying use-case driving this complaint that should be
> considered by the community either to induce a fix to the behavior or just
> to solicit suggestions for better alternatives?

While I agree the specific test case presented isn't terribly compelling,
it's not that hard to think of other scenarios where somebody might be
surprised at the presence of prefetching. Consider perhaps the
requirement "give me the next value from sequence s1 that's divisible
by ten". On its face this doesn't seem like a terrible implementation:

for s in select nextval('s1') from generate_series(1,10) loop
if s % 10 = 0 then exit loop; end if;
end loop;

This will do what's asked for but it will typically increment the sequence
several extra times due to internal prefetching. Perhaps the application
can tolerate that, perhaps not. With a user-defined volatile function,
the unwanted side effects might be quite unacceptable.

Anyway, I'm not hugely motivated to change the code to fix this, but
I think we at least ought to document it.

regards, tom lane