On Tue, Mar 13, 2018 at 05:04:04PM +0900, Michael Paquier wrote:
> Instead of doing what you are suggesting, why not moving
> InitXLogInsert() out of InitXLOGAccess() and change InitPostgres() so as
> the allocations for WAL inserts is done unconditionally?  This has
> the cost of also making this allocation even for backends which are
> started during recovery, still we are talking about allocating a couple
> of bytes in exchange of addressing completely all race conditions in
> this area.  InitXLogInsert() does not depend on any post-recovery data
> like ThisTimeLineId, so a split is possible.

I have been hacking things this way, and it seems to me that it takes
care of all this class of problems.  CreateCheckPoint() actually
mentions that InitXLogInsert() cannot be called in a critical section,
so the comments don't match the code.  I also think that we still want
to be able to use RecoveryInProgress() in critical sections to do
decision-making for the generation of WAL records.
--
Michael
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 47a6c4d895..cff238ee2a 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8049,6 +8049,9 @@ LocalSetXLogInsertAllowed(void)
 
 	/* Initialize as RecoveryInProgress() would do when switching state */
 	InitXLOGAccess();
+
+	/* Also initialize the working areas for constructing WAL records */
+	InitXLogInsert();
 }
 
 /*
@@ -8178,9 +8181,6 @@ InitXLOGAccess(void)
 	(void) GetRedoRecPtr();
 	/* Also update our copy of doPageWrites. */
 	doPageWrites = (Insert->fullPageWrites || Insert->forcePageWrites);
-
-	/* Also initialize the working areas for constructing WAL records */
-	InitXLogInsert();
 }
 
 /*
@@ -8582,11 +8582,11 @@ CreateCheckPoint(int flags)
 
 	/*
 	 * Initialize InitXLogInsert working areas before entering the critical
-	 * section.  Normally, this is done by the first call to
-	 * RecoveryInProgress() or LocalSetXLogInsertAllowed(), but when creating
-	 * an end-of-recovery checkpoint, the LocalSetXLogInsertAllowed call is
-	 * done below in a critical section, and InitXLogInsert cannot be called
-	 * in a critical section.
+	 * section.  Normally, this is done at backend startup or when calling
+	 * LocalSetXLogInsertAllowed(), but when creating an end-of-recovery
+	 * checkpoint, the LocalSetXLogInsertAllowed call is done below in a
+	 * critical section, and InitXLogInsert cannot be called in a critical
+	 * section.
 	 */
 	InitXLogInsert();
 
diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
index 28ff2f0979..4d20b9712f 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -452,6 +452,7 @@ AuxiliaryProcessMain(int argc, char *argv[])
 		case WalWriterProcess:
 			/* don't set signals, walwriter has its own agenda */
 			InitXLOGAccess();
+			InitXLogInsert();
 			WalWriterMain();
 			proc_exit(1);		/* should never return */
 
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index d8f45b3c43..8db377c1cf 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -618,6 +618,15 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username,
 	 */
 	if (IsUnderPostmaster)
 	{
+		/*
+		 * Initialize the working areas for constructing WAL records.
+		 * This is done even for a standby instance to avoid initialization
+		 * of this machinery after a promotion, which could happen in a
+		 * critical section and InitXLogInsert() cannot be called in such
+		 * a code path.
+		 */
+		InitXLogInsert();
+
 		/*
 		 * The postmaster already started the XLOG machinery, but we need to
 		 * call InitXLOGAccess(), if the system isn't in hot-standby mode.

Attachment: signature.asc
Description: PGP signature

Reply via email to