committing inside cursor loop
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
Attachments:
v1-0001-PL-pgSQL-Allow-committing-inside-cursor-loop.patchtext/plain; charset=UTF-8; name=v1-0001-PL-pgSQL-Allow-committing-inside-cursor-loop.patch; x-mac-creator=0; x-mac-type=0Download
From 649c84f49faa3f46e546ff9eb6d1b6e98483bdb8 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@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
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
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.
I haven't really read this patch, but this bit jumped out at me:
+ Inside a cursor loop, <command>ROLLBACK</command> is not allowed. (That
+ would have to roll back the cursor query, thus invalidating the loop.)Say what? That seems to translate into "we have lost the ability to
deal with errors". I don't think this is really what people are hoping
to get out of the transactional-procedure facility.
regards, tom lane
On 20 February 2018 at 14:11, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
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.
Amazingly clean, looks great.
I notice that PersistHoldablePortal() does ExecutorRewind().
In most cases, the cursor loop doesn't ever rewind. So it would be
good if we could pass a parameter that skips the rewind since it will
never be needed and causes a performance hit. What I imagine is we can
just persist the as-yet unfetched portion of the cursor from the
current row onwards, rather than rewind and store the whole cursor
result.
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 20 February 2018 at 14:45, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
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.I haven't really read this patch, but this bit jumped out at me:
+ Inside a cursor loop, <command>ROLLBACK</command> is not allowed. (That + would have to roll back the cursor query, thus invalidating the loop.)
Hmm, why?
Rollback would only invalidate the cursor if it hadn't yet hit a
Commit. But if you did a commit, then the cursor would become holdable
and you could happily continue reading through the loop even after the
rollback.
So if Commit changes a pinned portal into a holdable cursor, we just
make Rollback do that also. Obviously only for pinned portals, i.e.
the query/ies whose results we are currently looping through.
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, 20 Feb 2018 09:11:50 -0500
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
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.
Hi,
The patch still applies, tests passed. The code looks nice,
documentation and tests are there.
Although as was discussed before it seems inconsistent without ROLLBACK
support. There was a little discussion about it, but no replies. Maybe
the status of the patch should be changed to 'Waiting on author' until
the end of discussion.
--
---
Ildus Kurbangaliev
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
On 3/6/18 07:48, Ildus Kurbangaliev wrote:
Although as was discussed before it seems inconsistent without ROLLBACK
support. There was a little discussion about it, but no replies. Maybe
the status of the patch should be changed to 'Waiting on author' until
the end of discussion.
I'm wondering what the semantics of it should be.
For example, consider this:
drop table test1;
create table test1 (a int, b int);
insert into test1 values (1, 11), (2, 22), (3, 33);
do
language plpgsql
$$
declare
x int;
begin
for x in update test1 set b = b + 1 where b > 20 returning a loop
raise info 'x = %', x;
if x = 2 then
rollback;
end if;
end loop;
end;
$$;
The ROLLBACK call in the first loop iteration undoes the UPDATE command
that drives the loop. Is it then sensible to continue the loop?
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, 13 Mar 2018 11:08:36 -0400
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
On 3/6/18 07:48, Ildus Kurbangaliev wrote:
Although as was discussed before it seems inconsistent without
ROLLBACK support. There was a little discussion about it, but no
replies. Maybe the status of the patch should be changed to
'Waiting on author' until the end of discussion.I'm wondering what the semantics of it should be.
For example, consider this:
drop table test1;
create table test1 (a int, b int);
insert into test1 values (1, 11), (2, 22), (3, 33);do
language plpgsql
$$
declare
x int;
begin
for x in update test1 set b = b + 1 where b > 20 returning a loop
raise info 'x = %', x;
if x = 2 then
rollback;
end if;
end loop;
end;
$$;The ROLLBACK call in the first loop iteration undoes the UPDATE
command that drives the loop. Is it then sensible to continue the
loop?
I think that in the first place ROLLBACK was prohibited because of cases
like this, but it seems to safe to continue the loop when portal
strategy is PORTAL_ONE_SELECT.
--
---
Ildus Kurbangaliev
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
On 3/14/18 08:05, Ildus Kurbangaliev wrote:
The ROLLBACK call in the first loop iteration undoes the UPDATE
command that drives the loop. Is it then sensible to continue the
loop?I think that in the first place ROLLBACK was prohibited because of cases
like this, but it seems to safe to continue the loop when portal
strategy is PORTAL_ONE_SELECT.
Maybe, but even plain SELECT commands can have side effects.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 3/19/18 20:40, Peter Eisentraut wrote:
On 3/14/18 08:05, Ildus Kurbangaliev wrote:
The ROLLBACK call in the first loop iteration undoes the UPDATE
command that drives the loop. Is it then sensible to continue the
loop?I think that in the first place ROLLBACK was prohibited because of cases
like this, but it seems to safe to continue the loop when portal
strategy is PORTAL_ONE_SELECT.Maybe, but even plain SELECT commands can have side effects.
Here is an updated patch that supports the ROLLBACK case as well, and
prevents holding portals with a strategy other than PORTAL_ONE_SELECT.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
v2-0001-PL-pgSQL-Allow-committing-inside-cursor-loop.patchtext/plain; charset=UTF-8; name=v2-0001-PL-pgSQL-Allow-committing-inside-cursor-loop.patch; x-mac-creator=0; x-mac-type=0Download
From 1627af796da44d03f2d0d739250621be1c3244a3 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Mon, 26 Mar 2018 15:40:49 -0400
Subject: [PATCH v2] PL/pgSQL: Allow committing inside cursor loop
Previously, committing or aborting 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 or rollbacks. 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 or rollback, but is still removed
when returning to the main loop on error.
---
doc/src/sgml/plpgsql.sgml | 37 +++++-
src/backend/tcop/postgres.c | 2 +
src/backend/utils/mmgr/portalmem.c | 145 +++++++++++++++++----
src/include/utils/portal.h | 4 +
.../plpgsql/src/expected/plpgsql_transaction.out | 108 ++++++++++++++-
src/pl/plpgsql/src/pl_exec.c | 18 +--
src/pl/plpgsql/src/sql/plpgsql_transaction.sql | 67 ++++++++++
7 files changed, 333 insertions(+), 48 deletions(-)
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index 7ed926fd51..2043e644a9 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -3496,8 +3496,41 @@ <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> or
+ <command>ROLLBACK</command>. That means that the cursor is fully
+ evaluated at the first <command>COMMIT</command> or
+ <command>ROLLBACK</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>
+ Transaction commands are not allowed in cursor loops driven by commands
+ that are not read-only (for example <command>UPDATE
+ ... RETURNING</command>).
+ </para>
+
+ <para>
+ A transaction cannot be ended inside a block with exception handlers.
</para>
</sect1>
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index f7aa4a7484..6fc1cc272b 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -3938,6 +3938,8 @@ PostgresMain(int argc, char *argv[],
if (am_walsender)
WalSndErrorCleanup();
+ PortalErrorCleanup();
+
/*
* We can't release replication slots inside AbortTransaction() as we
* need to be able to start and abort transactions while having a slot
diff --git a/src/backend/utils/mmgr/portalmem.c b/src/backend/utils/mmgr/portalmem.c
index 75a6dde32b..74a1040d8a 100644
--- a/src/backend/utils/mmgr/portalmem.c
+++ b/src/backend/utils/mmgr/portalmem.c
@@ -620,6 +620,36 @@ PortalHashTableDeleteAll(void)
}
}
+/*
+ * "Hold" a portal. Prepare it for access by later transactions.
+ */
+static void
+HoldPortal(Portal portal)
+{
+ /*
+ * Note that PersistHoldablePortal() must release all resources
+ * used by the portal that are local to the creating transaction.
+ */
+ PortalCreateHoldStore(portal);
+ PersistHoldablePortal(portal);
+
+ /* drop cached plan reference, if any */
+ PortalReleaseCachedPlan(portal);
+
+ /*
+ * Any resources belonging to the portal will be released in the
+ * upcoming transaction-wide cleanup; the portal will no longer
+ * have its own resources.
+ */
+ portal->resowner = NULL;
+
+ /*
+ * Having successfully exported the holdable cursor, mark it as
+ * not belonging to this transaction.
+ */
+ portal->createSubid = InvalidSubTransactionId;
+ portal->activeSubid = InvalidSubTransactionId;
+}
/*
* Pre-commit processing for portals.
@@ -648,9 +678,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");
/*
@@ -684,29 +715,7 @@ PreCommit_Portals(bool isPrepare)
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot PREPARE a transaction that has created a cursor WITH HOLD")));
- /*
- * Note that PersistHoldablePortal() must release all resources
- * used by the portal that are local to the creating transaction.
- */
- PortalCreateHoldStore(portal);
- PersistHoldablePortal(portal);
-
- /* drop cached plan reference, if any */
- PortalReleaseCachedPlan(portal);
-
- /*
- * Any resources belonging to the portal will be released in the
- * upcoming transaction-wide cleanup; the portal will no longer
- * have its own resources.
- */
- portal->resowner = NULL;
-
- /*
- * Having successfully exported the holdable cursor, mark it as
- * not belonging to this transaction.
- */
- portal->createSubid = InvalidSubTransactionId;
- portal->activeSubid = InvalidSubTransactionId;
+ HoldPortal(portal);
/* Report we changed state */
result = true;
@@ -771,6 +780,14 @@ AtAbort_Portals(void)
if (portal->createSubid == InvalidSubTransactionId)
continue;
+ /*
+ * Do nothing to auto-held cursors. This is similar to the case of a
+ * cursor from a previous transaction, but it could also be that the
+ * cursor was auto-held in this transaction, so it wants to live on.
+ */
+ if (portal->autoHeld)
+ continue;
+
/*
* If it was created in the current transaction, we can't do normal
* shutdown on a READY portal either; it might refer to objects
@@ -834,8 +851,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 or
+ * auto-held ones.
+ */
+ if (portal->createSubid == InvalidSubTransactionId || portal->autoHeld)
{
Assert(portal->status != PORTAL_ACTIVE);
Assert(portal->resowner == NULL);
@@ -865,6 +885,32 @@ AtCleanup_Portals(void)
}
}
+/*
+ * Portal-related cleanup when we return to the main loop on error.
+ *
+ * This is different from the cleanup at transaction abort. Auto-held portals
+ * are cleaned up on error but not on transaction abort.
+ */
+void
+PortalErrorCleanup(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->autoHeld)
+ {
+ portal->portalPinned = false;
+ PortalDrop(portal, false);
+ }
+ }
+}
+
/*
* Pre-subcommit processing for portals.
*
@@ -1182,3 +1228,48 @@ ThereArePinnedPortals(void)
return false;
}
+
+/*
+ * Hold all pinned portals.
+ *
+ * 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->autoHeld)
+ {
+ /*
+ * Doing transaction control, especially abort, inside a cursor
+ * loop that is not read-only, for example using UPDATE
+ * ... RETURNING, has weird semantics issues. Also, this
+ * implementation wouldn't work, because such portals cannot be
+ * held. (The core grammar enforces that only SELECT statements
+ * can drive a cursor, but for example PL/pgSQL does not restrict
+ * it.)
+ */
+ if (portal->strategy != PORTAL_ONE_SELECT)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TRANSACTION_TERMINATION),
+ errmsg("cannot perform transaction commands inside a cursor loop that is not read-only")));
+
+ portal->autoHeld = true;
+ HoldPortal(portal);
+ }
+ }
+}
diff --git a/src/include/utils/portal.h b/src/include/utils/portal.h
index b903cb0fbe..1a760465b5 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 */
@@ -204,6 +206,7 @@ extern void EnablePortalManager(void);
extern bool PreCommit_Portals(bool isPrepare);
extern void AtAbort_Portals(void);
extern void AtCleanup_Portals(void);
+extern void PortalErrorCleanup(void);
extern void AtSubCommit_Portals(SubTransactionId mySubid,
SubTransactionId parentSubid,
ResourceOwner parentXactOwner);
@@ -232,5 +235,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 ce66487137..1ea63a685c 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
@@ -163,13 +199,79 @@ BEGIN
END LOOP;
END;
$$;
-ERROR: cannot abort transaction inside a cursor loop
+SELECT * FROM test1;
+ a | b
+---+---
+(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;
+$$;
+SELECT * FROM test1;
+ a | b
+---+---
+ 0 |
+ 2 |
+ 4 |
+(3 rows)
+
+SELECT * FROM pg_cursors;
+ name | statement | is_holdable | is_binary | is_scrollable | creation_time
+------+-----------+-------------+-----------+---------------+---------------
+(0 rows)
+
+-- rollback inside cursor loop
+TRUNCATE test1;
+DO LANGUAGE plpgsql $$
+DECLARE
+ r RECORD;
+BEGIN
+ FOR r IN UPDATE test2 SET x = x * 2 RETURNING x LOOP
+ INSERT INTO test1 (a) VALUES (r.x);
+ ROLLBACK;
+ END LOOP;
+END;
+$$;
+ERROR: cannot perform transaction commands inside a cursor loop that is not read-only
CONTEXT: PL/pgSQL function inline_code_block line 7 at ROLLBACK
SELECT * FROM test1;
a | b
---+---
(0 rows)
+SELECT * FROM test2;
+ x
+---
+ 0
+ 1
+ 2
+ 3
+ 4
+(5 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/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 38ea7a091f..7d3ba9241e 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -4642,14 +4642,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();
@@ -4668,14 +4661,7 @@ exec_stmt_commit(PLpgSQL_execstate *estate, PLpgSQL_stmt_commit *stmt)
static int
exec_stmt_rollback(PLpgSQL_execstate *estate, PLpgSQL_stmt_rollback *stmt)
{
- /*
- * Unlike the COMMIT case above, this might not make sense at all,
- * especially if the query driving the cursor loop has side effects.
- */
- if (ThereArePinnedPortals())
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_TRANSACTION_TERMINATION),
- errmsg("cannot abort transaction inside a cursor loop")));
+ HoldPinnedPortals();
SPI_rollback();
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..49d3435185 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,51 @@ 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;
+
+
+-- rollback inside cursor loop
+TRUNCATE test1;
+
+DO LANGUAGE plpgsql $$
+DECLARE
+ r RECORD;
+BEGIN
+ FOR r IN UPDATE test2 SET x = x * 2 RETURNING x LOOP
+ INSERT INTO test1 (a) VALUES (r.x);
+ ROLLBACK;
+ END LOOP;
+END;
+$$;
+
+SELECT * FROM test1;
+SELECT * FROM test2;
+
+SELECT * FROM pg_cursors;
+
-- commit inside block with exception handler
TRUNCATE test1;
base-commit: 64f85894ad2730fb1449a8e81dd8026604e9a546
--
2.16.3
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, passed
I have checked new version. Although I can miss something, the patch looks good to me.
The new status of this patch is: Ready for Committer
On 3/28/18 11:34, Ildus Kurbangaliev wrote:
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, passedI have checked new version. Although I can miss something, the patch looks good to me.
The new status of this patch is: Ready for Committer
Committed, thanks!
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
"Peter" == Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
Peter> Committed, thanks!
So... what is a pl/* that does _not_ use pinned cursors for cursor loops
supposed to do in this case?
--
Andrew (irc:RhodiumToad)
On 3/29/18 07:37, Andrew Gierth wrote:
"Peter" == Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
Peter> Committed, thanks!
So... what is a pl/* that does _not_ use pinned cursors for cursor loops
supposed to do in this case?
Other than maybe using pinned cursors, one would have to look at the
whole picture and see what makes sense.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
"Peter" == Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
So... what is a pl/* that does _not_ use pinned cursors for cursor
loops supposed to do in this case?
Peter> Other than maybe using pinned cursors, one would have to look at
Peter> the whole picture and see what makes sense.
Never mind, I was overlooking the obvious: explicitly specifying
CURSOR_OPT_HOLD is sufficient for me. (Can't really use pinned cursors
because they might not be unpinned fast enough due to timing of garbage
collection, which would lead to spurious errors.)
--
Andrew (irc:RhodiumToad)