From 7a6c80ffb7c610640d9cede8535ceb79a55ddb8f Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Mon, 14 Aug 2017 13:54:57 -0300
Subject: [PATCH v2 1/2] Release lwlocks in autovacuum launcher error handling
 path

For regular processes, lwlock release is handling via
AbortCurrentTransaction(), which autovacuum already does.  However,
autovacuum launcher sometimes obtains lock outside of any transaction,
in which case AbortCurrentTransaction is a no-op.  Continuing after
error recovery would block if we tried to obtain an lwlock that we
failed to release.

Reported-by: Robert Haas
Discussion: https://postgr.es/m/CA+TgmobQVbz4K_+RSmiM9HeRKpy3vS5xnbkL95gSEnWijzprKQ@mail.gmail.com
---
 src/backend/access/transam/xact.c     | 35 +++++++++++++++++++++++++++++++++++
 src/backend/bootstrap/bootstrap.c     |  4 +---
 src/backend/postmaster/autovacuum.c   |  7 ++++++-
 src/backend/postmaster/bgwriter.c     | 21 +++------------------
 src/backend/postmaster/checkpointer.c | 23 +++--------------------
 src/backend/postmaster/walwriter.c    | 22 +++-------------------
 src/backend/replication/walsender.c   |  7 +------
 src/include/access/xact.h             |  1 +
 8 files changed, 53 insertions(+), 67 deletions(-)

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index a8fbd043ae..34d15c75d6 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -3136,6 +3136,41 @@ AbortCurrentTransaction(void)
 }
 
 /*
+ *	AbortInAuxiliaryProcess
+ *
+ *	Cleanup and release resources during abort processing in auxiliary
+ *	processes.
+ */
+void
+AbortInAuxiliaryProcess(void)
+{
+	/*
+	 * These operations are really just a minimal subset of
+	 * AbortTransaction().  We don't have very many resources to worry about
+	 * in auxiliary processes, but we do have LWLocks, buffers, and temp files.
+	 */
+	LWLockReleaseAll();
+	ConditionVariableCancelSleep();
+	pgstat_report_wait_end();
+	AbortBufferIO();
+	UnlockBuffers();
+
+	/* buffer pins are released here: */
+	if (CurrentResourceOwner)
+	{
+		ResourceOwnerRelease(CurrentResourceOwner,
+							 RESOURCE_RELEASE_BEFORE_LOCKS,
+							 false, true);
+		/* we needn't bother with the other ResourceOwnerRelease phases */
+	}
+
+	AtEOXact_Buffers(false);
+	AtEOXact_SMgr();
+	AtEOXact_Files();
+	AtEOXact_HashTables(false);
+}
+
+/*
  *	PreventTransactionChain
  *
  *	This routine is to be called by statements that must not run inside
diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
index b3f0b3cc92..1d0b997234 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -543,9 +543,7 @@ bootstrap_signals(void)
 static void
 ShutdownAuxiliaryProcess(int code, Datum arg)
 {
-	LWLockReleaseAll();
-	ConditionVariableCancelSleep();
-	pgstat_report_wait_end();
+	AbortInAuxiliaryProcess();
 }
 
 /* ----------------------------------------------------------------
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 00b1e823af..9a42a766b3 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -521,8 +521,13 @@ AutoVacLauncherMain(int argc, char *argv[])
 		/* Report the error to the server log */
 		EmitErrorReport();
 
-		/* Abort the current transaction in order to recover */
+		/*
+		 * Aborting the current transaction releases process resources; do
+		 * generic abort processing for the cases were the launcher is not in a
+		 * transaction.
+		 */
 		AbortCurrentTransaction();
+		AbortInAuxiliaryProcess();
 
 		/*
 		 * Now return to normal top-level context and clear ErrorContext for
diff --git a/src/backend/postmaster/bgwriter.c b/src/backend/postmaster/bgwriter.c
index 9ad74ee977..2f6f9add61 100644
--- a/src/backend/postmaster/bgwriter.c
+++ b/src/backend/postmaster/bgwriter.c
@@ -38,6 +38,7 @@
 #include <sys/time.h>
 #include <unistd.h>
 
+#include "access/xact.h"
 #include "access/xlog.h"
 #include "access/xlog_internal.h"
 #include "libpq/pqsignal.h"
@@ -182,24 +183,8 @@ BackgroundWriterMain(void)
 		/* Report the error to the server log */
 		EmitErrorReport();
 
-		/*
-		 * These operations are really just a minimal subset of
-		 * AbortTransaction().  We don't have very many resources to worry
-		 * about in bgwriter, but we do have LWLocks, buffers, and temp files.
-		 */
-		LWLockReleaseAll();
-		ConditionVariableCancelSleep();
-		AbortBufferIO();
-		UnlockBuffers();
-		/* buffer pins are released here: */
-		ResourceOwnerRelease(CurrentResourceOwner,
-							 RESOURCE_RELEASE_BEFORE_LOCKS,
-							 false, true);
-		/* we needn't bother with the other ResourceOwnerRelease phases */
-		AtEOXact_Buffers(false);
-		AtEOXact_SMgr();
-		AtEOXact_Files();
-		AtEOXact_HashTables(false);
+		/* cleanup and release resources */
+		AbortInAuxiliaryProcess();
 
 		/*
 		 * Now return to normal top-level context and clear ErrorContext for
diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c
index e48ebd557f..e7291c38ef 100644
--- a/src/backend/postmaster/checkpointer.c
+++ b/src/backend/postmaster/checkpointer.c
@@ -41,6 +41,7 @@
 #include <time.h>
 #include <unistd.h>
 
+#include "access/xact.h"
 #include "access/xlog.h"
 #include "access/xlog_internal.h"
 #include "libpq/pqsignal.h"
@@ -264,26 +265,8 @@ CheckpointerMain(void)
 		/* Report the error to the server log */
 		EmitErrorReport();
 
-		/*
-		 * These operations are really just a minimal subset of
-		 * AbortTransaction().  We don't have very many resources to worry
-		 * about in checkpointer, but we do have LWLocks, buffers, and temp
-		 * files.
-		 */
-		LWLockReleaseAll();
-		ConditionVariableCancelSleep();
-		pgstat_report_wait_end();
-		AbortBufferIO();
-		UnlockBuffers();
-		/* buffer pins are released here: */
-		ResourceOwnerRelease(CurrentResourceOwner,
-							 RESOURCE_RELEASE_BEFORE_LOCKS,
-							 false, true);
-		/* we needn't bother with the other ResourceOwnerRelease phases */
-		AtEOXact_Buffers(false);
-		AtEOXact_SMgr();
-		AtEOXact_Files();
-		AtEOXact_HashTables(false);
+		/* cleanup and release resources */
+		AbortInAuxiliaryProcess();
 
 		/* Warn any waiting backends that the checkpoint failed. */
 		if (ckpt_active)
diff --git a/src/backend/postmaster/walwriter.c b/src/backend/postmaster/walwriter.c
index 7b89e02428..1498d3a53b 100644
--- a/src/backend/postmaster/walwriter.c
+++ b/src/backend/postmaster/walwriter.c
@@ -44,6 +44,7 @@
 #include <signal.h>
 #include <unistd.h>
 
+#include "access/xact.h"
 #include "access/xlog.h"
 #include "libpq/pqsignal.h"
 #include "miscadmin.h"
@@ -162,25 +163,8 @@ WalWriterMain(void)
 		/* Report the error to the server log */
 		EmitErrorReport();
 
-		/*
-		 * These operations are really just a minimal subset of
-		 * AbortTransaction().  We don't have very many resources to worry
-		 * about in walwriter, but we do have LWLocks, and perhaps buffers?
-		 */
-		LWLockReleaseAll();
-		ConditionVariableCancelSleep();
-		pgstat_report_wait_end();
-		AbortBufferIO();
-		UnlockBuffers();
-		/* buffer pins are released here: */
-		ResourceOwnerRelease(CurrentResourceOwner,
-							 RESOURCE_RELEASE_BEFORE_LOCKS,
-							 false, true);
-		/* we needn't bother with the other ResourceOwnerRelease phases */
-		AtEOXact_Buffers(false);
-		AtEOXact_SMgr();
-		AtEOXact_Files();
-		AtEOXact_HashTables(false);
+		/* cleanup and release resources */
+		AbortInAuxiliaryProcess();
 
 		/*
 		 * Now return to normal top-level context and clear ErrorContext for
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 03e1cf44de..1a62a174b0 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -290,9 +290,7 @@ InitWalSender(void)
 void
 WalSndErrorCleanup(void)
 {
-	LWLockReleaseAll();
-	ConditionVariableCancelSleep();
-	pgstat_report_wait_end();
+	AbortInAuxiliaryProcess();
 
 	if (sendFile >= 0)
 	{
@@ -300,9 +298,6 @@ WalSndErrorCleanup(void)
 		sendFile = -1;
 	}
 
-	if (MyReplicationSlot != NULL)
-		ReplicationSlotRelease();
-
 	ReplicationSlotCleanup();
 
 	replication_active = false;
diff --git a/src/include/access/xact.h b/src/include/access/xact.h
index ad5aad96df..4f8c6434e1 100644
--- a/src/include/access/xact.h
+++ b/src/include/access/xact.h
@@ -348,6 +348,7 @@ extern void ForceSyncCommit(void);
 extern void StartTransactionCommand(void);
 extern void CommitTransactionCommand(void);
 extern void AbortCurrentTransaction(void);
+extern void AbortInAuxiliaryProcess(void);
 extern void BeginTransactionBlock(void);
 extern bool EndTransactionBlock(void);
 extern bool PrepareTransactionBlock(char *gid);
-- 
2.11.0

