From: Andres Freund <andres@anarazel.de>
Date: Sun, 17 Sep 2017 08:00:39 +0000 (-0700)
Subject: Fix crash restart bug introduced in 8356753c212.
X-Git-Tag: REL_11_BETA1~1567
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=ec9e05b3c392ba9587f283507459737684539574;p=postgresql

Fix crash restart bug introduced in 8356753c212.

The bug was caused by not re-reading the control file during crash
recovery restarts, which lead to an attempt to pfree() shared memory
contents. The fix is to re-read the control file, which seems good
anyway.

It's unclear as of this moment, whether we want to keep the
refactoring introduced in the commit referenced above, or come up with
an alternative approach. But fixing the bug in the mean time seems
like a good idea regardless.

A followup commit will introduce regression test coverage for crash
restarts.

Reported-By: Tom Lane
Discussion: https://postgr.es/m/14134.1505572349@sss.pgh.pa.us
---

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index b8f648927a..96ebf32a58 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -4802,15 +4802,19 @@ check_wal_buffers(int *newval, void **extra, GucSource source)
 /*
  * Read the control file, set respective GUCs.
  *
- * This is to be called during startup, unless in bootstrap mode, where no
- * control file yet exists.  As there's no shared memory yet (its sizing can
- * depend on the contents of the control file!), first store data in local
- * memory. XLOGShemInit() will then copy it to shared memory later.
+ * This is to be called during startup, including a crash recovery cycle,
+ * unless in bootstrap mode, where no control file yet exists.  As there's no
+ * usable shared memory yet (its sizing can depend on the contents of the
+ * control file!), first store the contents in local memory. XLOGShemInit()
+ * will then copy it to shared memory later.
+ *
+ * reset just controls whether previous contents are to be expected (in the
+ * reset case, there's a dangling pointer into old shared memory), or not.
  */
 void
-LocalProcessControlFile(void)
+LocalProcessControlFile(bool reset)
 {
-	Assert(ControlFile == NULL);
+	Assert(reset || ControlFile == NULL);
 	ControlFile = palloc(sizeof(ControlFileData));
 	ReadControlFile();
 }
@@ -4884,20 +4888,13 @@ XLOGShmemInit(void)
 	}
 #endif
 
-	/*
-	 * Already have read control file locally, unless in bootstrap mode. Move
-	 * local version into shared memory.
-	 */
+
+	XLogCtl = (XLogCtlData *)
+		ShmemInitStruct("XLOG Ctl", XLOGShmemSize(), &foundXLog);
+
 	localControlFile = ControlFile;
 	ControlFile = (ControlFileData *)
 		ShmemInitStruct("Control File", sizeof(ControlFileData), &foundCFile);
-	if (localControlFile)
-	{
-		memcpy(ControlFile, localControlFile, sizeof(ControlFileData));
-		pfree(localControlFile);
-	}
-	XLogCtl = (XLogCtlData *)
-		ShmemInitStruct("XLOG Ctl", XLOGShmemSize(), &foundXLog);
 
 	if (foundCFile || foundXLog)
 	{
@@ -4908,10 +4905,23 @@ XLOGShmemInit(void)
 		WALInsertLocks = XLogCtl->Insert.WALInsertLocks;
 		LWLockRegisterTranche(LWTRANCHE_WAL_INSERT,
 							  "wal_insert");
+
+		if (localControlFile)
+			pfree(localControlFile);
 		return;
 	}
 	memset(XLogCtl, 0, sizeof(XLogCtlData));
 
+	/*
+	 * Already have read control file locally, unless in bootstrap mode. Move
+	 * contents into shared memory.
+	 */
+	if (localControlFile)
+	{
+		memcpy(ControlFile, localControlFile, sizeof(ControlFileData));
+		pfree(localControlFile);
+	}
+
 	/*
 	 * Since XLogCtlData contains XLogRecPtr fields, its sizeof should be a
 	 * multiple of the alignment for same, so no extra alignment padding is
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index e4f8f597c6..160b555294 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -951,7 +951,7 @@ PostmasterMain(int argc, char *argv[])
 	CreateDataDirLockFile(true);
 
 	/* read control file (error checking and contains config) */
-	LocalProcessControlFile();
+	LocalProcessControlFile(false);
 
 	/*
 	 * Initialize SSL library, if specified.
@@ -3829,6 +3829,10 @@ PostmasterStateMachine(void)
 		ResetBackgroundWorkerCrashTimes();
 
 		shmem_exit(1);
+
+		/* re-read control file into local memory */
+		LocalProcessControlFile(true);
+
 		reset_shared(PostPortNumber);
 
 		StartupPID = StartupDataBase();
@@ -4808,8 +4812,11 @@ SubPostmasterMain(int argc, char *argv[])
 	/* Read in remaining GUC variables */
 	read_nondefault_variables();
 
-	/* (re-)read control file (contains config) */
-	LocalProcessControlFile();
+	/*
+	 * (re-)read control file, as it contains config. The postmaster will
+	 * already have read this, but this process doesn't know about that.
+	 */
+	LocalProcessControlFile(false);
 
 	/*
 	 * Reload any libraries that were preloaded by the postmaster.  Since we
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 46b662266b..dfd52b3c87 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -3718,7 +3718,7 @@ PostgresMain(int argc, char *argv[],
 		CreateDataDirLockFile(false);
 
 		/* read control file (error checking and contains config ) */
-		LocalProcessControlFile();
+		LocalProcessControlFile(false);
 
 		/* Initialize MaxBackends (if under postmaster, was done already) */
 		InitializeMaxBackends();
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index e0635ab4e6..7213af0e81 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -261,7 +261,7 @@ extern XLogRecPtr GetFakeLSNForUnloggedRel(void);
 extern Size XLOGShmemSize(void);
 extern void XLOGShmemInit(void);
 extern void BootStrapXLOG(void);
-extern void LocalProcessControlFile(void);
+extern void LocalProcessControlFile(bool reset);
 extern void StartupXLOG(void);
 extern void ShutdownXLOG(int code, Datum arg);
 extern void InitXLOGAccess(void);