Avoid misbehavior when persisting a non-stable cursor.

PersistHoldablePortal has long assumed that it should store the
entire output of the query-to-be-persisted, which requires rewinding
and re-reading the output.  This is problematic if the query is not
stable: we might get different row contents, or even a different
number of rows, which'd confuse the cursor state mightily.

In the case where the cursor is NO SCROLL, this is very easy to
solve: just store the remaining query output, without any rewinding,
and tweak the portal's cursor state to match.  Aside from removing
the semantic problem, this could be significantly more efficient
than storing the whole output.

If the cursor is scrollable, there's not much we can do, but it
was already the case that scrolling a volatile query's result was
pretty unsafe.  We can just document more clearly that getting
correct results from that is not guaranteed.

There are already prohibitions in place on using SCROLL with
FOR UPDATE/SHARE, which is one way for a SELECT query to have
non-stable results.  We could imagine prohibiting SCROLL when
the query contains volatile functions, but that would be
expensive to enforce.  Moreover, it could break applications
that work just fine, if they have functions that are in fact
stable but the user neglected to mark them so.  So settle for
documenting the hazard.

While this problem has existed in some guise for a long time,
it got a lot worse in v11, which introduced the possibility
of persisting plpgsql cursors (perhaps implicit ones) even
when they violate the rules for what can be marked WITH HOLD.
Hence, I've chosen to back-patch to v11 but not further.

Per bug #17050 from Алексей Булгаков.

Discussion: https://postgr.es/m/17050-f77aa827dc85247c@postgresql.org

diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index b562bfd9c3005633135150455c4e61c6b7d71139..5c546f630fe0db9b38a41ccc9242b3f3e0ac831e 100644 (file)
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -3033,6 +3033,15 @@ DECLARE
      is said to be unbound since it is not bound to
      any particular query.
     
+
+    
+     The SCROLL option cannot be used when the cursor's
+     query uses FOR UPDATE/SHARE.  Also, it is
+     best to use NO SCROLL with a query that involves
+     volatile functions.  The implementation of SCROLL
+     assumes that re-reading the query's output will give consistent
+     results, which a volatile function might not do.
+    
    
 
    

diff --git a/doc/src/sgml/ref/declare.sgml b/doc/src/sgml/ref/declare.sgml
index d6177dcd9c44ed2cfab5ffb7caccda82ee5e97a7..951dfa982baf7164013953b42535858e396613b8 100644 (file)
--- a/doc/src/sgml/ref/declare.sgml
+++ b/doc/src/sgml/ref/declare.sgml
@@ -228,12 +228,14 @@ DECLARE name [ BINARY ] [ INSENSITI
 
    
     
-     Scrollable and WITH HOLD cursors may give unexpected
+     Scrollable cursors may give unexpected
      results if they invoke any volatile functions (see 
      linkend="xfunc-volatility"/>).  When a previously fetched row is
      re-fetched, the functions might be re-executed, perhaps leading to
-     results different from the first time.  One workaround for such cases
-     is to declare the cursor WITH HOLD and commit the
+     results different from the first time.  It's best to
+     specify NO SCROLL for a query involving volatile
+     functions.  If that is not practical, one workaround
+     is to declare the cursor SCROLL WITH HOLD and commit the
      transaction before reading any rows from it.  This will force the
      entire output of the cursor to be materialized in temporary storage,
      so that volatile functions are executed exactly once for each row.


diff --git a/src/backend/commands/portalcmds.c b/src/backend/commands/portalcmds.c

index f65529ba6ad1347500d2d2f8cb68609bf1bf9129..4bd7ec1ba449932df1fd80de17eb8ec27acfaae8 100644 (file)

--- a/src/backend/commands/portalcmds.c
+++ b/src/backend/commands/portalcmds.c
@@ -374,10 +374,23 @@ PersistHoldablePortal(Portal portal)
        PushActiveSnapshot(queryDesc->snapshot);
 
        /*
-        * Rewind the executor: we need to store the entire result set in the
-        * tuplestore, so that subsequent backward FETCHs can be processed.
+        * If the portal is marked scrollable, we need to store the entire
+        * result set in the tuplestore, so that subsequent backward FETCHs
+        * can be processed.  Otherwise, store only the not-yet-fetched rows.
+        * (The latter is not only more efficient, but avoids semantic
+        * problems if the query's output isn't stable.)
         */
-       ExecutorRewind(queryDesc);
+       if (portal->cursorOptions & CURSOR_OPT_SCROLL)
+       {
+           ExecutorRewind(queryDesc);
+       }
+       else
+       {
+           /* We must reset the cursor state as though at start of query */
+           portal->atStart = true;
+           portal->atEnd = false;
+           portal->portalPos = 0;
+       }
 
        /*
         * Change the destination to output to the tuplestore.  Note we tell


diff --git a/src/pl/plpgsql/src/expected/plpgsql_transaction.out b/src/pl/plpgsql/src/expected/plpgsql_transaction.out

index e205a1e00227c9aa778da7bd9d5774af51ab191a..918cc0913e6b9368240ec054a0b1e3f2adfda0f1 100644 (file)

--- a/src/pl/plpgsql/src/expected/plpgsql_transaction.out
+++ b/src/pl/plpgsql/src/expected/plpgsql_transaction.out
@@ -335,6 +335,57 @@ SELECT * FROM pg_cursors;
 ------+-----------+-------------+-----------+---------------+---------------
 (0 rows)
 
+-- interaction of FOR UPDATE cursor with subsequent updates (bug #17050)
+TRUNCATE test1;
+INSERT INTO test1 VALUES (1,'one'), (2,'two'), (3,'three');
+DO LANGUAGE plpgsql $$
+DECLARE
+    l_cur CURSOR FOR SELECT a FROM test1 ORDER BY 1 FOR UPDATE;
+BEGIN
+    FOR r IN l_cur LOOP
+      UPDATE test1 SET b = b || ' ' || b WHERE a = r.a;
+      COMMIT;
+    END LOOP;
+END;
+$$;
+SELECT * FROM test1;
+ a |      b      
+---+-------------
+ 1 | one one
+ 2 | two two
+ 3 | three three
+(3 rows)
+
+SELECT * FROM pg_cursors;
+ name | statement | is_holdable | is_binary | is_scrollable | creation_time 
+------+-----------+-------------+-----------+---------------+---------------
+(0 rows)
+
+-- like bug #17050, but with implicit cursor
+TRUNCATE test1;
+INSERT INTO test1 VALUES (1,'one'), (2,'two'), (3,'three');
+DO LANGUAGE plpgsql $$
+DECLARE r RECORD;
+BEGIN
+    FOR r IN SELECT a FROM test1 FOR UPDATE LOOP
+      UPDATE test1 SET b = b || ' ' || b WHERE a = r.a;
+      COMMIT;
+    END LOOP;
+END;
+$$;
+SELECT * FROM test1;
+ a |      b      
+---+-------------
+ 1 | one one
+ 2 | two two
+ 3 | three three
+(3 rows)
+
+SELECT * FROM pg_cursors;
+ name | statement | is_holdable | is_binary | is_scrollable | creation_time 
+------+-----------+-------------+-----------+---------------+---------------
+(0 rows)
+
 -- commit inside block with exception handler
 TRUNCATE test1;
 DO LANGUAGE plpgsql $$


diff --git a/src/pl/plpgsql/src/sql/plpgsql_transaction.sql b/src/pl/plpgsql/src/sql/plpgsql_transaction.sql

index 94fd406b7a346bea1112752dcf9167bde0d66934..cc26788b9ae7cfe4ca808480f6c634b8bf10f428 100644 (file)

--- a/src/pl/plpgsql/src/sql/plpgsql_transaction.sql
+++ b/src/pl/plpgsql/src/sql/plpgsql_transaction.sql
@@ -273,6 +273,47 @@ SELECT * FROM test2;
 SELECT * FROM pg_cursors;
 
 
+-- interaction of FOR UPDATE cursor with subsequent updates (bug #17050)
+TRUNCATE test1;
+
+INSERT INTO test1 VALUES (1,'one'), (2,'two'), (3,'three');
+
+DO LANGUAGE plpgsql $$
+DECLARE
+    l_cur CURSOR FOR SELECT a FROM test1 ORDER BY 1 FOR UPDATE;
+BEGIN
+    FOR r IN l_cur LOOP
+      UPDATE test1 SET b = b || ' ' || b WHERE a = r.a;
+      COMMIT;
+    END LOOP;
+END;
+$$;
+
+SELECT * FROM test1;
+
+SELECT * FROM pg_cursors;
+
+
+-- like bug #17050, but with implicit cursor
+TRUNCATE test1;
+
+INSERT INTO test1 VALUES (1,'one'), (2,'two'), (3,'three');
+
+DO LANGUAGE plpgsql $$
+DECLARE r RECORD;
+BEGIN
+    FOR r IN SELECT a FROM test1 FOR UPDATE LOOP
+      UPDATE test1 SET b = b || ' ' || b WHERE a = r.a;
+      COMMIT;
+    END LOOP;
+END;
+$$;
+
+SELECT * FROM test1;
+
+SELECT * FROM pg_cursors;
+
+
 -- commit inside block with exception handler
 TRUNCATE test1;
 

diff --git a/src/backend/commands/portalcmds.c b/src/backend/commands/portalcmds.c
index f65529ba6ad1347500d2d2f8cb68609bf1bf9129..4bd7ec1ba449932df1fd80de17eb8ec27acfaae8 100644 (file)
--- a/src/backend/commands/portalcmds.c
+++ b/src/backend/commands/portalcmds.c
@@ -374,10 +374,23 @@ PersistHoldablePortal(Portal portal)
        PushActiveSnapshot(queryDesc->snapshot);
 
        /*
-        * Rewind the executor: we need to store the entire result set in the
-        * tuplestore, so that subsequent backward FETCHs can be processed.
+        * If the portal is marked scrollable, we need to store the entire
+        * result set in the tuplestore, so that subsequent backward FETCHs
+        * can be processed.  Otherwise, store only the not-yet-fetched rows.
+        * (The latter is not only more efficient, but avoids semantic
+        * problems if the query's output isn't stable.)
         */
-       ExecutorRewind(queryDesc);
+       if (portal->cursorOptions & CURSOR_OPT_SCROLL)
+       {
+           ExecutorRewind(queryDesc);
+       }
+       else
+       {
+           /* We must reset the cursor state as though at start of query */
+           portal->atStart = true;
+           portal->atEnd = false;
+           portal->portalPos = 0;
+       }
 
        /*
         * Change the destination to output to the tuplestore.  Note we tell

diff --git a/src/pl/plpgsql/src/expected/plpgsql_transaction.out b/src/pl/plpgsql/src/expected/plpgsql_transaction.out
index e205a1e00227c9aa778da7bd9d5774af51ab191a..918cc0913e6b9368240ec054a0b1e3f2adfda0f1 100644 (file)
--- a/src/pl/plpgsql/src/expected/plpgsql_transaction.out
+++ b/src/pl/plpgsql/src/expected/plpgsql_transaction.out
@@ -335,6 +335,57 @@ SELECT * FROM pg_cursors;
 ------+-----------+-------------+-----------+---------------+---------------
 (0 rows)
 
+-- interaction of FOR UPDATE cursor with subsequent updates (bug #17050)
+TRUNCATE test1;
+INSERT INTO test1 VALUES (1,'one'), (2,'two'), (3,'three');
+DO LANGUAGE plpgsql $$
+DECLARE
+    l_cur CURSOR FOR SELECT a FROM test1 ORDER BY 1 FOR UPDATE;
+BEGIN
+    FOR r IN l_cur LOOP
+      UPDATE test1 SET b = b || ' ' || b WHERE a = r.a;
+      COMMIT;
+    END LOOP;
+END;
+$$;
+SELECT * FROM test1;
+ a |      b      
+---+-------------
+ 1 | one one
+ 2 | two two
+ 3 | three three
+(3 rows)
+
+SELECT * FROM pg_cursors;
+ name | statement | is_holdable | is_binary | is_scrollable | creation_time 
+------+-----------+-------------+-----------+---------------+---------------
+(0 rows)
+
+-- like bug #17050, but with implicit cursor
+TRUNCATE test1;
+INSERT INTO test1 VALUES (1,'one'), (2,'two'), (3,'three');
+DO LANGUAGE plpgsql $$
+DECLARE r RECORD;
+BEGIN
+    FOR r IN SELECT a FROM test1 FOR UPDATE LOOP
+      UPDATE test1 SET b = b || ' ' || b WHERE a = r.a;
+      COMMIT;
+    END LOOP;
+END;
+$$;
+SELECT * FROM test1;
+ a |      b      
+---+-------------
+ 1 | one one
+ 2 | two two
+ 3 | three three
+(3 rows)
+
+SELECT * FROM pg_cursors;
+ name | statement | is_holdable | is_binary | is_scrollable | creation_time 
+------+-----------+-------------+-----------+---------------+---------------
+(0 rows)
+
 -- commit inside block with exception handler
 TRUNCATE test1;
 DO LANGUAGE plpgsql $$

diff --git a/src/pl/plpgsql/src/sql/plpgsql_transaction.sql b/src/pl/plpgsql/src/sql/plpgsql_transaction.sql
index 94fd406b7a346bea1112752dcf9167bde0d66934..cc26788b9ae7cfe4ca808480f6c634b8bf10f428 100644 (file)
--- a/src/pl/plpgsql/src/sql/plpgsql_transaction.sql
+++ b/src/pl/plpgsql/src/sql/plpgsql_transaction.sql
@@ -273,6 +273,47 @@ SELECT * FROM test2;
 SELECT * FROM pg_cursors;
 
 
+-- interaction of FOR UPDATE cursor with subsequent updates (bug #17050)
+TRUNCATE test1;
+
+INSERT INTO test1 VALUES (1,'one'), (2,'two'), (3,'three');
+
+DO LANGUAGE plpgsql $$
+DECLARE
+    l_cur CURSOR FOR SELECT a FROM test1 ORDER BY 1 FOR UPDATE;
+BEGIN
+    FOR r IN l_cur LOOP
+      UPDATE test1 SET b = b || ' ' || b WHERE a = r.a;
+      COMMIT;
+    END LOOP;
+END;
+$$;
+
+SELECT * FROM test1;
+
+SELECT * FROM pg_cursors;
+
+
+-- like bug #17050, but with implicit cursor
+TRUNCATE test1;
+
+INSERT INTO test1 VALUES (1,'one'), (2,'two'), (3,'three');
+
+DO LANGUAGE plpgsql $$
+DECLARE r RECORD;
+BEGIN
+    FOR r IN SELECT a FROM test1 FOR UPDATE LOOP
+      UPDATE test1 SET b = b || ' ' || b WHERE a = r.a;
+      COMMIT;
+    END LOOP;
+END;
+$$;
+
+SELECT * FROM test1;
+
+SELECT * FROM pg_cursors;
+
+
 -- commit inside block with exception handler
 TRUNCATE test1;