Here is a patch that allows COMMIT inside cursor loops in PL/pgSQL.  As
alluded to in earlier threads, this is done by converting such cursors
to holdable automatically.  A special flag "auto-held" marks such
cursors, so we know to clean them up on exceptions.

This is currently only for PL/pgSQL, but the same technique could be
applied easily to other PLs.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 649c84f49faa3f46e546ff9eb6d1b6e98483bdb8 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pete...@gmx.net>
Date: Tue, 20 Feb 2018 08:52:23 -0500
Subject: [PATCH v1] PL/pgSQL: Allow committing inside cursor loop

Previously, committing inside a cursor loop was prohibited because that
would close and remove the cursor.  To allow that, automatically convert
such cursors to holdable cursors so they survive commits.  Portals now
have a new state "auto-held", which means they have been converted
automatically from pinned.  An auto-held portal is kept on transaction
commit but is still removed on transaction abort.
---
 doc/src/sgml/plpgsql.sgml                          | 35 +++++++++-
 src/backend/utils/mmgr/portalmem.c                 | 49 ++++++++++++--
 src/include/utils/portal.h                         |  3 +
 .../plpgsql/src/expected/plpgsql_transaction.out   | 74 +++++++++++++++++++++-
 src/pl/plpgsql/src/pl_exec.c                       |  9 +--
 src/pl/plpgsql/src/sql/plpgsql_transaction.sql     | 47 ++++++++++++++
 6 files changed, 199 insertions(+), 18 deletions(-)

diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index c1e3c6a19d..6ac72cc5ac 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -3480,8 +3480,39 @@ <title>Transaction Management</title>
    </para>
 
    <para>
-    A transaction cannot be ended inside a loop over a query result, nor
-    inside a block with exception handlers.
+    Special considerations apply to cursor loops.  Consider this example:
+<programlisting>
+CREATE PROCEDURE transaction_test2()
+LANGUAGE plpgsql
+AS $$
+DECLARE
+    r RECORD;
+BEGIN
+    FOR r IN SELECT * FROM test2 ORDER BY x LOOP
+        INSERT INTO test1 (a) VALUES (r.x);
+        COMMIT;
+    END LOOP;
+END;
+$$;
+
+CALL transaction_test2();
+</programlisting>
+    Normally, cursors are automatically closed at transaction commit.
+    However, a cursor created as part of a loop like this is automatically
+    converted to a holdable cursor by the first <command>COMMIT</command>.
+    That means that the cursor is fully evaluated at the first
+    <command>COMMIT</command> rather than row by row.  The cursor is still
+    removed automatically after the loop, so this is mostly invisible to the
+    user.
+   </para>
+
+   <para>
+    Inside a cursor loop, <command>ROLLBACK</command> is not allowed.  (That
+    would have to roll back the cursor query, thus invalidating the loop.)
+   </para>
+
+   <para>
+    A transaction cannot be ended inside a block with exception handlers.
    </para>
   </sect1>
 
diff --git a/src/backend/utils/mmgr/portalmem.c 
b/src/backend/utils/mmgr/portalmem.c
index 75a6dde32b..983a6d4436 100644
--- a/src/backend/utils/mmgr/portalmem.c
+++ b/src/backend/utils/mmgr/portalmem.c
@@ -648,9 +648,10 @@ PreCommit_Portals(bool isPrepare)
 
                /*
                 * There should be no pinned portals anymore. Complain if 
someone
-                * leaked one.
+                * leaked one. Auto-held portals are allowed; we assume that 
whoever
+                * pinned them is managing them.
                 */
-               if (portal->portalPinned)
+               if (portal->portalPinned && !portal->autoHeld)
                        elog(ERROR, "cannot commit while a portal is pinned");
 
                /*
@@ -766,9 +767,10 @@ AtAbort_Portals(void)
                        MarkPortalFailed(portal);
 
                /*
-                * Do nothing else to cursors held over from a previous 
transaction.
+                * Do nothing else to cursors held over from a previous 
transaction,
+                * except auto-held ones.
                 */
-               if (portal->createSubid == InvalidSubTransactionId)
+               if (portal->createSubid == InvalidSubTransactionId && 
!portal->autoHeld)
                        continue;
 
                /*
@@ -834,8 +836,11 @@ AtCleanup_Portals(void)
                if (portal->status == PORTAL_ACTIVE)
                        continue;
 
-               /* Do nothing to cursors held over from a previous transaction 
*/
-               if (portal->createSubid == InvalidSubTransactionId)
+               /*
+                * Do nothing to cursors held over from a previous transaction, 
except
+                * auto-held ones.
+                */
+               if (portal->createSubid == InvalidSubTransactionId && 
!portal->autoHeld)
                {
                        Assert(portal->status != PORTAL_ACTIVE);
                        Assert(portal->resowner == NULL);
@@ -1182,3 +1187,35 @@ ThereArePinnedPortals(void)
 
        return false;
 }
+
+/*
+ * Convert all pinned portals to holdable ones.  (The actual "holding" will be
+ * done later by transaction commit.)
+ *
+ * A procedural language implementation that uses pinned portals for its
+ * internally generated cursors can call this in its COMMIT command to convert
+ * those cursors to held cursors, so that they survive the transaction end.
+ * We mark those portals as "auto-held" so that exception exit knows to clean
+ * them up.  (In normal, non-exception code paths, the PL needs to clean those
+ * portals itself, since transaction end won't do it anymore, but that should
+ * be normal practice anyway.)
+ */
+void
+HoldPinnedPortals(void)
+{
+       HASH_SEQ_STATUS status;
+       PortalHashEnt *hentry;
+
+       hash_seq_init(&status, PortalHashTable);
+
+       while ((hentry = (PortalHashEnt *) hash_seq_search(&status)) != NULL)
+       {
+               Portal          portal = hentry->portal;
+
+               if (portal->portalPinned)
+               {
+                       portal->cursorOptions = portal->cursorOptions | 
CURSOR_OPT_HOLD;
+                       portal->autoHeld = true;
+               }
+       }
+}
diff --git a/src/include/utils/portal.h b/src/include/utils/portal.h
index b903cb0fbe..dc088d0deb 100644
--- a/src/include/utils/portal.h
+++ b/src/include/utils/portal.h
@@ -147,6 +147,8 @@ typedef struct PortalData
        /* Status data */
        PortalStatus status;            /* see above */
        bool            portalPinned;   /* a pinned portal can't be dropped */
+       bool            autoHeld;               /* was automatically converted 
from pinned to
+                                                                * held (see 
HoldPinnedPortals()) */
 
        /* If not NULL, Executor is active; call ExecutorEnd eventually: */
        QueryDesc  *queryDesc;          /* info needed for executor invocation 
*/
@@ -232,5 +234,6 @@ extern void PortalCreateHoldStore(Portal portal);
 extern void PortalHashTableDeleteAll(void);
 extern bool ThereAreNoReadyPortals(void);
 extern bool ThereArePinnedPortals(void);
+extern void HoldPinnedPortals(void);
 
 #endif                                                 /* PORTAL_H */
diff --git a/src/pl/plpgsql/src/expected/plpgsql_transaction.out 
b/src/pl/plpgsql/src/expected/plpgsql_transaction.out
index 8ec22c646c..1ecec6ba06 100644
--- a/src/pl/plpgsql/src/expected/plpgsql_transaction.out
+++ b/src/pl/plpgsql/src/expected/plpgsql_transaction.out
@@ -144,11 +144,47 @@ BEGIN
     END LOOP;
 END;
 $$;
-ERROR:  committing inside a cursor loop is not supported
-CONTEXT:  PL/pgSQL function inline_code_block line 7 at COMMIT
 SELECT * FROM test1;
  a | b 
 ---+---
+ 0 | 
+ 1 | 
+ 2 | 
+ 3 | 
+ 4 | 
+(5 rows)
+
+-- check that this doesn't leak a holdable portal
+SELECT * FROM pg_cursors;
+ name | statement | is_holdable | is_binary | is_scrollable | creation_time 
+------+-----------+-------------+-----------+---------------+---------------
+(0 rows)
+
+-- error in cursor loop with commit
+TRUNCATE test1;
+DO LANGUAGE plpgsql $$
+DECLARE
+    r RECORD;
+BEGIN
+    FOR r IN SELECT * FROM test2 ORDER BY x LOOP
+        INSERT INTO test1 (a) VALUES (12/(r.x-2));
+        COMMIT;
+    END LOOP;
+END;
+$$;
+ERROR:  division by zero
+CONTEXT:  SQL statement "INSERT INTO test1 (a) VALUES (12/(r.x-2))"
+PL/pgSQL function inline_code_block line 6 at SQL statement
+SELECT * FROM test1;
+  a  | b 
+-----+---
+  -6 | 
+ -12 | 
+(2 rows)
+
+SELECT * FROM pg_cursors;
+ name | statement | is_holdable | is_binary | is_scrollable | creation_time 
+------+-----------+-------------+-----------+---------------+---------------
 (0 rows)
 
 -- rollback inside cursor loop
@@ -170,6 +206,40 @@ SELECT * FROM test1;
 ---+---
 (0 rows)
 
+SELECT * FROM pg_cursors;
+ name | statement | is_holdable | is_binary | is_scrollable | creation_time 
+------+-----------+-------------+-----------+---------------+---------------
+(0 rows)
+
+-- first commit then rollback inside cursor loop
+TRUNCATE test1;
+DO LANGUAGE plpgsql $$
+DECLARE
+    r RECORD;
+BEGIN
+    FOR r IN SELECT * FROM test2 ORDER BY x LOOP
+        INSERT INTO test1 (a) VALUES (r.x);
+        IF r.x % 2 = 0 THEN
+            COMMIT;
+        ELSE
+            ROLLBACK;
+        END IF;
+    END LOOP;
+END;
+$$;
+ERROR:  cannot abort transaction inside a cursor loop
+CONTEXT:  PL/pgSQL function inline_code_block line 10 at ROLLBACK
+SELECT * FROM test1;
+ a | b 
+---+---
+ 0 | 
+(1 row)
+
+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/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index eae51e316a..e70a8a1cb1 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -4522,14 +4522,7 @@ exec_stmt_close(PLpgSQL_execstate *estate, 
PLpgSQL_stmt_close *stmt)
 static int
 exec_stmt_commit(PLpgSQL_execstate *estate, PLpgSQL_stmt_commit *stmt)
 {
-       /*
-        * XXX This could be implemented by converting the pinned portals to
-        * holdable ones and organizing the cleanup separately.
-        */
-       if (ThereArePinnedPortals())
-               ereport(ERROR,
-                               (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-                                errmsg("committing inside a cursor loop is not 
supported")));
+       HoldPinnedPortals();
 
        SPI_commit();
        SPI_start_transaction();
diff --git a/src/pl/plpgsql/src/sql/plpgsql_transaction.sql 
b/src/pl/plpgsql/src/sql/plpgsql_transaction.sql
index 02ee735079..b230de4d64 100644
--- a/src/pl/plpgsql/src/sql/plpgsql_transaction.sql
+++ b/src/pl/plpgsql/src/sql/plpgsql_transaction.sql
@@ -135,6 +135,28 @@ CREATE TABLE test2 (x int);
 
 SELECT * FROM test1;
 
+-- check that this doesn't leak a holdable portal
+SELECT * FROM pg_cursors;
+
+
+-- error in cursor loop with commit
+TRUNCATE test1;
+
+DO LANGUAGE plpgsql $$
+DECLARE
+    r RECORD;
+BEGIN
+    FOR r IN SELECT * FROM test2 ORDER BY x LOOP
+        INSERT INTO test1 (a) VALUES (12/(r.x-2));
+        COMMIT;
+    END LOOP;
+END;
+$$;
+
+SELECT * FROM test1;
+
+SELECT * FROM pg_cursors;
+
 
 -- rollback inside cursor loop
 TRUNCATE test1;
@@ -152,6 +174,31 @@ CREATE TABLE test2 (x int);
 
 SELECT * FROM test1;
 
+SELECT * FROM pg_cursors;
+
+
+-- first commit then rollback inside cursor loop
+TRUNCATE test1;
+
+DO LANGUAGE plpgsql $$
+DECLARE
+    r RECORD;
+BEGIN
+    FOR r IN SELECT * FROM test2 ORDER BY x LOOP
+        INSERT INTO test1 (a) VALUES (r.x);
+        IF r.x % 2 = 0 THEN
+            COMMIT;
+        ELSE
+            ROLLBACK;
+        END IF;
+    END LOOP;
+END;
+$$;
+
+SELECT * FROM test1;
+
+SELECT * FROM pg_cursors;
+
 
 -- commit inside block with exception handler
 TRUNCATE test1;

base-commit: 9a44a26b65d3d36867267624b76d3dea3dc4f6f6
-- 
2.16.2

Reply via email to