From 649c84f49faa3f46e546ff9eb6d1b6e98483bdb8 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut 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 @@ Transaction Management - 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: + +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(); + + 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 COMMIT. + That means that the cursor is fully evaluated at the first + COMMIT rather than row by row. The cursor is still + removed automatically after the loop, so this is mostly invisible to the + user. + + + + Inside a cursor loop, ROLLBACK is not allowed. (That + would have to roll back the cursor query, thus invalidating the loop.) + + + + A transaction cannot be ended inside a block with exception handlers. 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