I think I've found a reasonable fix for this.
The problem arises with the combination of CALL with output parameters
and doing a COMMIT inside the procedure. When a CALL has output
parameters, the portal uses the strategy PORTAL_UTIL_SELECT instead of
PORTAL_MULTI_QUERY. Using PORTAL_UTIL_SELECT causes the portal's
snapshot to be registered with the current resource
owner (portal->holdSnapshot); see
9ee1cf04ab6bcefe03a11837b53f29ca9dc24c7a for the reason.
Normally, PortalDrop() unregisters the snapshot. If not, then
ResourceOwnerRelease() will print a warning about a snapshot leak on
transaction commit. A transaction commit normally drops all
portals (PreCommit_Portals()), except the active portal. So in case of
the active portal, we need to manually release the snapshot to avoid the
warning.
PreCommit_Portals() already contains some code that deals specially with
the active portal versus resource owners, so it seems reasonable to add
there.
I think this problem could theoretically apply to any
multiple-transaction utility command. For example, if we had a variant
of VACUUM that returned tuples, it would produce the same warning.
(This patch would fix that as well.)
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From d91f42f024afa7bc2764045b615296a87c4033b8 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pete...@gmx.net>
Date: Thu, 23 Aug 2018 15:13:48 +0200
Subject: [PATCH] Fix snapshot leak warning for some procedures
The problem arises with the combination of CALL with output parameters
and doing a COMMIT inside the procedure. When a CALL has output
parameters, the portal uses the strategy PORTAL_UTIL_SELECT instead of
PORTAL_MULTI_QUERY. Using PORTAL_UTIL_SELECT causes the portal's
snapshot to be registered with the current resource
owner (portal->holdSnapshot); see
9ee1cf04ab6bcefe03a11837b53f29ca9dc24c7a for the reason.
Normally, PortalDrop() unregisters the snapshot. If not, then
ResourceOwnerRelease() will print a warning about a snapshot leak on
transaction commit. A transaction commit normally drops all
portals (PreCommit_Portals()), except the active portal. So in case of
the active portal, we need to manually release the snapshot to avoid the
warning.
Reported-by: Prabhat Sahu <prabhat.s...@enterprisedb.com>
---
src/backend/utils/mmgr/portalmem.c | 14 +++++++--
.../src/expected/plpgsql_transaction.out | 30 +++++++++++++++++++
.../plpgsql/src/sql/plpgsql_transaction.sql | 25 ++++++++++++++++
3 files changed, 67 insertions(+), 2 deletions(-)
diff --git a/src/backend/utils/mmgr/portalmem.c
b/src/backend/utils/mmgr/portalmem.c
index 04ea32f49f..d34cab0eb8 100644
--- a/src/backend/utils/mmgr/portalmem.c
+++ b/src/backend/utils/mmgr/portalmem.c
@@ -689,13 +689,23 @@ PreCommit_Portals(bool isPrepare)
/*
* Do not touch active portals --- this can only happen in the
case of
- * a multi-transaction utility command, such as VACUUM.
+ * a multi-transaction utility command, such as VACUUM, or a
commit in
+ * a procedure.
*
* Note however that any resource owner attached to such a
portal is
- * still going to go away, so don't leave a dangling pointer.
+ * still going to go away, so don't leave a dangling pointer.
Also
+ * unregister any snapshots held by the portal, mainly to avoid
+ * snapshot leak warnings from ResourceOwnerRelease().
*/
if (portal->status == PORTAL_ACTIVE)
{
+ if (portal->holdSnapshot)
+ {
+ if (portal->resowner)
+
UnregisterSnapshotFromOwner(portal->holdSnapshot,
+
portal->resowner);
+ portal->holdSnapshot = NULL;
+ }
portal->resowner = NULL;
continue;
}
diff --git a/src/pl/plpgsql/src/expected/plpgsql_transaction.out
b/src/pl/plpgsql/src/expected/plpgsql_transaction.out
index 0b5a039b89..77a83adab5 100644
--- a/src/pl/plpgsql/src/expected/plpgsql_transaction.out
+++ b/src/pl/plpgsql/src/expected/plpgsql_transaction.out
@@ -463,6 +463,36 @@ SELECT * FROM test2;
42
(1 row)
+-- Test transaction in procedure with output parameters. This uses a
+-- different portal strategy and different code paths in pquery.c.
+CREATE PROCEDURE transaction_test10a(INOUT x int)
+LANGUAGE plpgsql
+AS $$
+BEGIN
+ x := x + 1;
+ COMMIT;
+END;
+$$;
+CALL transaction_test10a(10);
+ x
+----
+ 11
+(1 row)
+
+CREATE PROCEDURE transaction_test10b(INOUT x int)
+LANGUAGE plpgsql
+AS $$
+BEGIN
+ x := x - 1;
+ ROLLBACK;
+END;
+$$;
+CALL transaction_test10b(10);
+ x
+---
+ 9
+(1 row)
+
DROP TABLE test1;
DROP TABLE test2;
DROP TABLE test3;
diff --git a/src/pl/plpgsql/src/sql/plpgsql_transaction.sql
b/src/pl/plpgsql/src/sql/plpgsql_transaction.sql
index 236db9bf2b..0ed9ab873a 100644
--- a/src/pl/plpgsql/src/sql/plpgsql_transaction.sql
+++ b/src/pl/plpgsql/src/sql/plpgsql_transaction.sql
@@ -387,6 +387,31 @@ CREATE PROCEDURE transaction_test9()
SELECT * FROM test2;
+-- Test transaction in procedure with output parameters. This uses a
+-- different portal strategy and different code paths in pquery.c.
+CREATE PROCEDURE transaction_test10a(INOUT x int)
+LANGUAGE plpgsql
+AS $$
+BEGIN
+ x := x + 1;
+ COMMIT;
+END;
+$$;
+
+CALL transaction_test10a(10);
+
+CREATE PROCEDURE transaction_test10b(INOUT x int)
+LANGUAGE plpgsql
+AS $$
+BEGIN
+ x := x - 1;
+ ROLLBACK;
+END;
+$$;
+
+CALL transaction_test10b(10);
+
+
DROP TABLE test1;
DROP TABLE test2;
DROP TABLE test3;
--
2.18.0
