At Thu, 2 Feb 2017 08:46:11 +0900, Michael Paquier <michael.paqu...@gmail.com>
wrote in <cab7npqr6vq7aikck1ao3_mpvvn4v4zknjfq2oawfqpaephd...@mail.gmail.com>
> On Thu, Feb 2, 2017 at 2:14 AM, Fujii Masao <masao.fu...@gmail.com> wrote:
> > The lwlock would be released when an exception occurs, so I don't think
> > that TRY-CATCH is necessary here. Or it's necessary for another reason?
>
> + PG_CATCH();
> + {
> + LWLockRelease(LogicalRepLauncherLock);
> + PG_RE_THROW();
> + }
> + PG_END_TRY();
> Just to do that, a TRY/CATCH block looks like an overkill to me. Why
> not just call LWLockRelease in the ERROR and return code paths?
I though the same first. The modification at the "if (wrconn =="
is the remains of that. It is reverted inthe attached patch.
Then, the reason for the TRY-CATCH cluase is that I found that
some functions called from there can throw exceptions.
logicalrep_worker_stop and replorigin_drop have ereport in its path.
load_library apparently can throw exception.
(walrcv_(libpq_) functions don't seeem to.)
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From d0ca653bb2aa776742a2e7a697b02794b1ad66d9 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyot...@lab.ntt.co.jp>
Date: Thu, 2 Feb 2017 11:33:40 +0900
Subject: [PATCH] Fix DROP SUBSCRIPTION's lock leak.
DROP SUBSCRIPTION acquires a lock on LogicalRepLauncherLock but never
releases. This fixes it.
---
src/backend/commands/subscriptioncmds.c | 86 ++++++++++++++++++---------------
1 file changed, 48 insertions(+), 38 deletions(-)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 5de9999..223eea4 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -511,52 +511,62 @@ DropSubscription(DropSubscriptionStmt *stmt)
/* Protect against launcher restarting the worker. */
LWLockAcquire(LogicalRepLauncherLock, LW_EXCLUSIVE);
- /* Kill the apply worker so that the slot becomes accessible. */
- logicalrep_worker_stop(subid);
-
- /* Remove the origin tracking if exists. */
- snprintf(originname, sizeof(originname), "pg_%u", subid);
- originid = replorigin_by_name(originname, true);
- if (originid != InvalidRepOriginId)
- replorigin_drop(originid);
-
- /* If the user asked to not drop the slot, we are done mow.*/
- if (!stmt->drop_slot)
- {
- heap_close(rel, NoLock);
- return;
- }
-
/*
- * Otherwise drop the replication slot at the publisher node using
- * the replication connection.
+ * Some functions called here can throw exceptions. we must release
+ * LogicalRepLauncherLock for the case.
*/
- load_file("libpqwalreceiver", false);
+ PG_TRY();
+ {
+ /* Kill the apply worker so that the slot becomes accessible. */
+ logicalrep_worker_stop(subid);
- initStringInfo(&cmd);
- appendStringInfo(&cmd, "DROP_REPLICATION_SLOT \"%s\"", slotname);
+ /* Remove the origin tracking if exists. */
+ snprintf(originname, sizeof(originname), "pg_%u", subid);
+ originid = replorigin_by_name(originname, true);
+ if (originid != InvalidRepOriginId)
+ replorigin_drop(originid);
- wrconn = walrcv_connect(conninfo, true, subname, &err);
- if (wrconn == NULL)
- ereport(ERROR,
- (errmsg("could not connect to publisher when attempting to "
- "drop the replication slot \"%s\"", slotname),
- errdetail("The error was: %s", err)));
+ /* Do the follwoing only if the user asked to actually drop the slot */
+ if (stmt->drop_slot)
+ {
+ /*
+ * Drop the replication slot at the publisher node using the
+ * replication connection.
+ */
+ load_file("libpqwalreceiver", false);
- if (!walrcv_command(wrconn, cmd.data, &err))
- ereport(ERROR,
- (errmsg("could not drop the replication slot \"%s\" on publisher",
- slotname),
- errdetail("The error was: %s", err)));
- else
- ereport(NOTICE,
- (errmsg("dropped replication slot \"%s\" on publisher",
- slotname)));
+ initStringInfo(&cmd);
+ appendStringInfo(&cmd, "DROP_REPLICATION_SLOT \"%s\"", slotname);
+
+ wrconn = walrcv_connect(conninfo, true, subname, &err);
+ if (wrconn == NULL)
+ ereport(ERROR,
+ (errmsg("could not connect to publisher when attempting to "
+ "drop the replication slot \"%s\"", slotname),
+ errdetail("The error was: %s", err)));
- walrcv_disconnect(wrconn);
+ else if (!walrcv_command(wrconn, cmd.data, &err))
+ ereport(ERROR,
+ (errmsg("could not drop the replication slot \"%s\" on publisher",
+ slotname),
+ errdetail("The error was: %s", err)));
+
+ ereport(NOTICE,
+ (errmsg("dropped replication slot \"%s\" on publisher",
+ slotname)));
- pfree(cmd.data);
+ walrcv_disconnect(wrconn);
+ pfree(cmd.data);
+ }
+ }
+ PG_CATCH();
+ {
+ LWLockRelease(LogicalRepLauncherLock);
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
+ LWLockRelease(LogicalRepLauncherLock);
heap_close(rel, NoLock);
}
--
2.9.2
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
