From d91f42f024afa7bc2764045b615296a87c4033b8 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut 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 --- 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