Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

On Thu, 2009-06-25 at 17:35 +0300, Heikki Linnakangas wrote:
> Heikki Linnakangas wrote:
> > Hmm, what happens when the startup process performs a write, and
> > bgwriter is not running? Do the fsync requests queue up in the shmem
> > queue until the end of recovery when bgwriter is launched? I guess I'll
> > have to try it out...
> 
> Oh dear, doesn't look good. The startup process has a pendingOpsTable of
> its own. bgwriter won't fsync() files that the startup process has
> written itself. That needs to be fixed, or you can lose data when an
> archive recovery crashes after a restartpoint.

Yes, that's what I see also. Patch attached.

-- 
 Simon Riggs           www.2ndQuadrant.com
 PostgreSQL Training, Services and Support

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Tom Lane wrote:
> Heikki Linnakangas  writes:
>> Heikki Linnakangas wrote:
>>> Hmm, what happens when the startup process performs a write, and
>>> bgwriter is not running? Do the fsync requests queue up in the shmem
>>> queue until the end of recovery when bgwriter is launched? I guess I'll
>>> have to try it out...
> 
>> Oh dear, doesn't look good. The startup process has a pendingOpsTable of
>> its own. bgwriter won't fsync() files that the startup process has
>> written itself. That needs to be fixed, or you can lose data when an
>> archive recovery crashes after a restartpoint.
> 
> Ouch.  I'm beginning to think that the best thing is to temporarily
> revert the change that made bgwriter active during recovery.  It's
> obviously not been adequately thought through or tested.

That was my first thought too, but unfortunately we now rely on bgwriter
to perform restartpoints :-(.

I came up with the attached patch, which includes Simon's patch to have
all fsync requests forwarded to bgwriter during archive recovery. To fix
the startup checkpoint issue, startup process requests a forced
restartpoint, which will flush any fsync requests bgwriter has
accumulated, before doing the actual checkpoint in the startup process.
This is completely untested still, but does anyone immediately see any
more problems?

-- 
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 4fd9d41..5114664 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -5156,6 +5156,7 @@ StartupXLOG(void)
 	XLogRecord *record;
 	uint32		freespace;
 	TransactionId oldestActiveXID;
+	bool		bgwriterLaunched = false;
 
 	XLogCtl->SharedRecoveryInProgress = true;
 
@@ -5472,7 +5473,11 @@ StartupXLOG(void)
 			 * process in addition to postmaster!
 			 */
 			if (InArchiveRecovery && IsUnderPostmaster)
+			{
+				SetForwardFsyncRequests();
 				SendPostmasterSignal(PMSIGNAL_RECOVERY_STARTED);
+				bgwriterLaunched = true;
+			}
 
 			/*
 			 * main redo apply loop
@@ -5742,7 +5747,16 @@ StartupXLOG(void)
 		 * assigning a new TLI, using a shutdown checkpoint allows us to have
 		 * the rule that TLI only changes in shutdown checkpoints, which
 		 * allows some extra error checking in xlog_redo.
+		 *
+		 * If bgwriter is active, we can't do the necessary fsyncs in the
+		 * startup process because bgwriter has accumulated fsync requests
+		 * into its private memory. So we request a last forced restart point,
+		 * which will flush them to disk, before performing the actual
+		 * checkpoint.
 		 */
+		if (bgwriterLaunched)
+			RequestCheckpoint(CHECKPOINT_IMMEDIATE | CHECKPOINT_FORCE |
+							  CHECKPOINT_WAIT);
 		CreateCheckPoint(CHECKPOINT_IS_SHUTDOWN | CHECKPOINT_IMMEDIATE);
 
 		/*
@@ -6219,27 +6233,11 @@ CreateCheckPoint(int flags)
 
 	/*
 	 * Acquire CheckpointLock to ensure only one checkpoint happens at a time.
-	 * During normal operation, bgwriter is the only process that creates
-	 * checkpoints, but at the end of archive recovery, the bgwriter can be
-	 * busy creating a restartpoint while the startup process tries to perform
-	 * the startup checkpoint.
+	 * (This is just pro forma, since in the present system structure there is
+	 * only one process that is allowed to issue checkpoints at any given
+	 * time.)
 	 */
-	if (!LWLockConditionalAcquire(CheckpointLock, LW_EXCLUSIVE))
-	{
-		Assert(InRecovery);
-
-		/*
-		 * A restartpoint is in progress. Wait until it finishes. This can
-		 * cause an extra restartpoint to be performed, but that's OK because
-		 * we're just about to perform a checkpoint anyway. Flushing the
-		 * buffers in this restartpoint can take some time, but that time is
-		 * saved from the upcoming checkpoint so the net effect is zero.
-		 */
-		ereport(DEBUG2, (errmsg("hurrying in-progress restartpoint")));
-		RequestCheckpoint(CHECKPOINT_IMMEDIATE | CHECKPOINT_WAIT);
-
-		LWLockAcquire(CheckpointLock, LW_EXCLUSIVE);
-	}
+	LWLockAcquire(CheckpointLock, LW_EXCLUSIVE);
 
 	/*
 	 * Prepare to accumulate statistics.
@@ -6660,8 +6658,9 @@ CreateRestartPoint(int flags)
 	 * restartpoint. It's assumed that flushing the buffers will do that as a
 	 * side-effect.
 	 */
-	if (XLogRecPtrIsInvalid(lastCheckPointRecPtr) ||
-		XLByteLE(lastCheckPoint.redo, ControlFile->checkPointCopy.redo))
+	if (!(flags & CHECKPOINT_FORCE) &&
+		(XLogRecPtrIsInvalid(lastCheckPointRecPtr) ||
+		 XLByteLE(lastCheckPoint.redo, ControlFile->checkPointCopy.redo)))
 	{
 		XLogRecPtr	InvalidXLogRecPtr = {0, 0};
 
diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index d42e86a..b01b2ab 100644
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -204,6 +204,21 @@ mdinit(void)
 }
 
 /*
+ * In archive recovery, we rely on bgwriter to do fsyncs(), but we don't
+ * know that we do archive recovery at process startup when pendingOpsTable
+ * has already been created. Calling this function drops pendingOpsTable
+ * and causes any subsequent requests to be forwarded to bgwriter.
+ */
+void
+SetForwardFsyncRequests(void)
+{
+	/* Perform any pending ops we may have queued up */
+	if (pendingOpsTable)
+		mdsync();
+	pendingOpsTable = NULL;
+}
+
+/*
  *	mdexists() -- Does the physical file exist?
  *
  * Note: this will return true for lingering files, with pending deletions
diff --git a/src/include/storage/smgr.h b/src/include/storage/smgr.h
index 7556b14..fd79d7b 100644
--- a/src/include/storage/smgr.h
+++ b/src/include/storage/smgr.h
@@ -109,6 +109,7 @@ extern void mdpreckpt(void);
 extern void mdsync(void);
 extern void mdpostckpt(void);
 
+extern void SetForwardFsyncRequests(void);
 extern void RememberFsyncRequest(RelFileNode rnode, ForkNumber forknum,
 					 BlockNumber segno);
 extern void ForgetRelationFsyncRequests(RelFileNode rnode, ForkNumber forknum);

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Simon Riggs wrote:
> On Thu, 2009-06-25 at 12:43 -0400, Tom Lane wrote:
> 
>> What about "revert the patch"?
> 
> That's probably just as dangerous.

I don't feel comfortable either reverting such a big patch at last
minute. Would need a fair amount of testing to make sure that the
revertion is correct and that no patches committed after the patch are
now broken.

I'm testing the attached patch at the moment. It's the same as the
previous one, with the elog() in mdsync() issue fixed.

-- 
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 4fd9d41..274369f 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -5156,6 +5156,7 @@ StartupXLOG(void)
 	XLogRecord *record;
 	uint32		freespace;
 	TransactionId oldestActiveXID;
+	bool		bgwriterLaunched = false;
 
 	XLogCtl->SharedRecoveryInProgress = true;
 
@@ -5472,7 +5473,11 @@ StartupXLOG(void)
 			 * process in addition to postmaster!
 			 */
 			if (InArchiveRecovery && IsUnderPostmaster)
+			{
+				SetForwardFsyncRequests();
 				SendPostmasterSignal(PMSIGNAL_RECOVERY_STARTED);
+				bgwriterLaunched = true;
+			}
 
 			/*
 			 * main redo apply loop
@@ -5742,7 +5747,17 @@ StartupXLOG(void)
 		 * assigning a new TLI, using a shutdown checkpoint allows us to have
 		 * the rule that TLI only changes in shutdown checkpoints, which
 		 * allows some extra error checking in xlog_redo.
+		 *
+		 * If bgwriter is active, we can't do the necessary fsyncs in the
+		 * startup process because bgwriter has accumulated fsync requests
+		 * into its private memory. So we request a last forced restart point,
+		 * which will flush them to disk, before performing the actual
+		 * checkpoint. It's safe to flush the buffers first, because no other
+		 * process can do (WAL-logged) changes to data pages in between.
 		 */
+		if (bgwriterLaunched)
+			RequestCheckpoint(CHECKPOINT_IMMEDIATE | CHECKPOINT_FORCE |
+							  CHECKPOINT_WAIT);
 		CreateCheckPoint(CHECKPOINT_IS_SHUTDOWN | CHECKPOINT_IMMEDIATE);
 
 		/*
@@ -6219,27 +6234,11 @@ CreateCheckPoint(int flags)
 
 	/*
 	 * Acquire CheckpointLock to ensure only one checkpoint happens at a time.
-	 * During normal operation, bgwriter is the only process that creates
-	 * checkpoints, but at the end of archive recovery, the bgwriter can be
-	 * busy creating a restartpoint while the startup process tries to perform
-	 * the startup checkpoint.
+	 * (This is just pro forma, since in the present system structure there is
+	 * only one process that is allowed to issue checkpoints at any given
+	 * time.)
 	 */
-	if (!LWLockConditionalAcquire(CheckpointLock, LW_EXCLUSIVE))
-	{
-		Assert(InRecovery);
-
-		/*
-		 * A restartpoint is in progress. Wait until it finishes. This can
-		 * cause an extra restartpoint to be performed, but that's OK because
-		 * we're just about to perform a checkpoint anyway. Flushing the
-		 * buffers in this restartpoint can take some time, but that time is
-		 * saved from the upcoming checkpoint so the net effect is zero.
-		 */
-		ereport(DEBUG2, (errmsg("hurrying in-progress restartpoint")));
-		RequestCheckpoint(CHECKPOINT_IMMEDIATE | CHECKPOINT_WAIT);
-
-		LWLockAcquire(CheckpointLock, LW_EXCLUSIVE);
-	}
+	LWLockAcquire(CheckpointLock, LW_EXCLUSIVE);
 
 	/*
 	 * Prepare to accumulate statistics.
@@ -6660,8 +6659,9 @@ CreateRestartPoint(int flags)
 	 * restartpoint. It's assumed that flushing the buffers will do that as a
 	 * side-effect.
 	 */
-	if (XLogRecPtrIsInvalid(lastCheckPointRecPtr) ||
-		XLByteLE(lastCheckPoint.redo, ControlFile->checkPointCopy.redo))
+	if (!(flags & CHECKPOINT_FORCE) &&
+		(XLogRecPtrIsInvalid(lastCheckPointRecPtr) ||
+		 XLByteLE(lastCheckPoint.redo, ControlFile->checkPointCopy.redo)))
 	{
 		XLogRecPtr	InvalidXLogRecPtr = {0, 0};
 
diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index d42e86a..56824d5 100644
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -204,6 +204,21 @@ mdinit(void)
 }
 
 /*
+ * In archive recovery, we rely on bgwriter to do fsyncs(), but we don't
+ * know that we do archive recovery at process startup when pendingOpsTable
+ * has already been created. Calling this function drops pendingOpsTable
+ * and causes any subsequent requests to be forwarded to bgwriter.
+ */
+void
+SetForwardFsyncRequests(void)
+{
+	/* Perform any pending ops we may have queued up */
+	if (pendingOpsTable)
+		mdsync();
+	pendingOpsTable = NULL;
+}
+
+/*
  *	mdexists() -- Does the physical file exist?
  *
  * Note: this will return true for lingering files, with pending deletions
@@ -908,9 +923,18 @@ mdsync(void)
 	/*
 	 * This is only called during checkpoints, and checkpoints should only
 	 * occur in processes that have created a pendingOpsTable.
+	 *
+	 * Startup process performing a startup checkpoint after archive recovery
+	 * is an exception. It has no pendingOpsTable, but that's OK because it
+	 * has requested bgwriter to perform a restartpoint before the checkpoint
+	 * which does mdsync() instead.
 	 */
 	if (!pendingOpsTable)
+	{
+		if (InRecovery)
+			return;
 		elog(ERROR, "cannot sync without a pendingOpsTable");
+	}
 
 	/*
 	 * If we are in the bgwriter, the sync had better include all fsync
diff --git a/src/include/storage/smgr.h b/src/include/storage/smgr.h
index 7556b14..fd79d7b 100644
--- a/src/include/storage/smgr.h
+++ b/src/include/storage/smgr.h
@@ -109,6 +109,7 @@ extern void mdpreckpt(void);
 extern void mdsync(void);
 extern void mdpostckpt(void);
 
+extern void SetForwardFsyncRequests(void);
 extern void RememberFsyncRequest(RelFileNode rnode, ForkNumber forknum,
 					 BlockNumber segno);
 extern void ForgetRelationFsyncRequests(RelFileNode rnode, ForkNumber forknum);

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Tom Lane wrote:
> Heikki Linnakangas  writes:
>> Simon Riggs wrote:
>>> On Thu, 2009-06-25 at 12:43 -0400, Tom Lane wrote:
>>>> What about "revert the patch"?
>>> That's probably just as dangerous.
> 
>> I don't feel comfortable either reverting such a big patch at last
>> minute.
> 
> Yeah, I'm not very happy with that either.  However, I've still got no
> confidence in anything proposed so far.
> 
>> I'm testing the attached patch at the moment. It's the same as the
>> previous one, with the elog() in mdsync() issue fixed.
> 
> This seems like a kluge on top of a hack.  Can't we have the bgwriter
> do the final checkpoint instead?

Here's a patch taking that approach, and I think it's better than the
previous  one. I was afraid we would lose robustness if we have to set
the shared state as "out of recovery" before requesting the checkpoint,
but we can use the same trick we were using in startup process and set
LocalRecoveryInProgress=false before setting the shared variable. I
introduced a new CHECKPOINT_IS_STARTUP flag, which is otherwise
identical to CHECKPOINT_IS_SHUTDOWN, but in a startup checkpoint
CreateCheckPoint() excpects to be called while recovery is still active,
and sets LocalRecoveryInProgress=false. It also tells bgwriter that it
needs to do a checkpoint instead of a restartpoint, even though recovery
is still in progress.

-- 
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 4fd9d41..18c47aa 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -5156,6 +5156,7 @@ StartupXLOG(void)
 	XLogRecord *record;
 	uint32		freespace;
 	TransactionId oldestActiveXID;
+	bool		bgwriterLaunched;
 
 	XLogCtl->SharedRecoveryInProgress = true;
 
@@ -5472,7 +5473,11 @@ StartupXLOG(void)
 			 * process in addition to postmaster!
 			 */
 			if (InArchiveRecovery && IsUnderPostmaster)
+			{
+				SetForwardFsyncRequests();
 				SendPostmasterSignal(PMSIGNAL_RECOVERY_STARTED);
+				bgwriterLaunched = true;
+			}
 
 			/*
 			 * main redo apply loop
@@ -5709,12 +5714,6 @@ StartupXLOG(void)
 	/* Pre-scan prepared transactions to find out the range of XIDs present */
 	oldestActiveXID = PrescanPreparedTransactions();
 
-	/*
-	 * Allow writing WAL for us, so that we can create a checkpoint record.
-	 * But not yet for other backends!
-	 */
-	LocalRecoveryInProgress = false;
-
 	if (InRecovery)
 	{
 		int			rmid;
@@ -5743,7 +5742,11 @@ StartupXLOG(void)
 		 * the rule that TLI only changes in shutdown checkpoints, which
 		 * allows some extra error checking in xlog_redo.
 		 */
-		CreateCheckPoint(CHECKPOINT_IS_SHUTDOWN | CHECKPOINT_IMMEDIATE);
+		if (bgwriterLaunched)
+			RequestCheckpoint(CHECKPOINT_IS_STARTUP | CHECKPOINT_IMMEDIATE |
+							  CHECKPOINT_WAIT);
+		else
+			CreateCheckPoint(CHECKPOINT_IS_STARTUP | CHECKPOINT_IMMEDIATE);
 
 		/*
 		 * And finally, execute the recovery_end_command, if any.
@@ -5806,7 +5809,7 @@ StartupXLOG(void)
 	}
 
 	/*
-	 * All done. Allow others to write WAL.
+	 * All done. Allow backends to write WAL.
 	 */
 	XLogCtl->SharedRecoveryInProgress = false;
 }
@@ -6123,12 +6126,13 @@ LogCheckpointStart(int flags, bool restartpoint)
 	 * the main message, but what about all the flags?
 	 */
 	if (restartpoint)
-		msg = "restartpoint starting:%s%s%s%s%s%s";
+		msg = "restartpoint starting:%s%s%s%s%s%s%s";
 	else
-		msg = "checkpoint starting:%s%s%s%s%s%s";
+		msg = "checkpoint starting:%s%s%s%s%s%s%s";
 
 	elog(LOG, msg,
 		 (flags & CHECKPOINT_IS_SHUTDOWN) ? " shutdown" : "",
+		 (flags & CHECKPOINT_IS_STARTUP) ? " startup" : "",
 		 (flags & CHECKPOINT_IMMEDIATE) ? " immediate" : "",
 		 (flags & CHECKPOINT_FORCE) ? " force" : "",
 		 (flags & CHECKPOINT_WAIT) ? " wait" : "",
@@ -6190,10 +6194,12 @@ LogCheckpointEnd(bool restartpoint)
  *
  * flags is a bitwise OR of the following:
  *	CHECKPOINT_IS_SHUTDOWN: checkpoint is for database shutdown.
+ *	CHECKPOINT_IS_STARTUP: checkpoint is for database startup.
  *	CHECKPOINT_IMMEDIATE: finish the checkpoint ASAP,
  *		ignoring checkpoint_completion_target parameter.
  *	CHECKPOINT_FORCE: force a checkpoint even if no XLOG activity has occured
- *		since the last one (implied by CHECKPOINT_IS_SHUTDOWN).
+ *		since the last one (implied by CHECKPOINT_IS_SHUTDOWN and
+ *		CHECKPOINT_IS_STARTUP).
  *
  * Note: flags contains other bits, of interest here only for logging purposes.
  * In particular note that this routine is synchronous and does not pay
@@ -6202,7 +6208,7 @@ LogCheckpointEnd(bool restartpoint)
 void
 CreateCheckPoint(int flags)
 {
-	bool		shutdown = (flags & CHECKPOINT_IS_SHUTDOWN) != 0;
+	bool		shutdown;
 	CheckPoint	checkPoint;
 	XLogRecPtr	recptr;
 	XLogCtlInsert *Insert = &XLogCtl->Insert;
@@ -6213,35 +6219,40 @@ CreateCheckPoint(int flags)
 	TransactionId *inCommitXids;
 	int			nInCommit;
 
-	/* shouldn't happen */
-	if (RecoveryInProgress())
-		elog(ERROR, "can't create a checkpoint during recovery");
+	/*
+	 * A startup checkpoint is really a shutdown checkpoint, just issued at
+	 * a different time.
+	 */
+	shutdown = (flags & (CHECKPOINT_IS_SHUTDOWN | CHECKPOINT_IS_STARTUP)) != 0;
 
 	/*
-	 * Acquire CheckpointLock to ensure only one checkpoint happens at a time.
-	 * During normal operation, bgwriter is the only process that creates
-	 * checkpoints, but at the end of archive recovery, the bgwriter can be
-	 * busy creating a restartpoint while the startup process tries to perform
-	 * the startup checkpoint.
+	 * A startup checkpoint is created before anyone else is allowed to
+	 * write WAL. To allow us to write the checkpoint record, set
+	 * LocalRecoveryInProgress to false. This lets us write WAL, but others
+	 * are still not allowed to do so.
 	 */
-	if (!LWLockConditionalAcquire(CheckpointLock, LW_EXCLUSIVE))
+	if (flags & CHECKPOINT_IS_STARTUP)
 	{
-		Assert(InRecovery);
-
-		/*
-		 * A restartpoint is in progress. Wait until it finishes. This can
-		 * cause an extra restartpoint to be performed, but that's OK because
-		 * we're just about to perform a checkpoint anyway. Flushing the
-		 * buffers in this restartpoint can take some time, but that time is
-		 * saved from the upcoming checkpoint so the net effect is zero.
-		 */
-		ereport(DEBUG2, (errmsg("hurrying in-progress restartpoint")));
-		RequestCheckpoint(CHECKPOINT_IMMEDIATE | CHECKPOINT_WAIT);
-
-		LWLockAcquire(CheckpointLock, LW_EXCLUSIVE);
+		Assert(RecoveryInProgress());
+		LocalRecoveryInProgress = false;
+		InitXLOGAccess();
+	}
+	else
+	{
+		/* shouldn't happen */
+		if (RecoveryInProgress())
+			elog(ERROR, "can't create a checkpoint during recovery");
 	}
 
 	/*
+	 * Acquire CheckpointLock to ensure only one checkpoint happens at a time.
+	 * (This is just pro forma, since in the present system structure there is
+	 * only one process that is allowed to issue checkpoints at any given
+	 * time.)
+	 */
+	LWLockAcquire(CheckpointLock, LW_EXCLUSIVE);
+
+	/*
 	 * Prepare to accumulate statistics.
 	 *
 	 * Note: because it is possible for log_checkpoints to change while a
@@ -6298,7 +6309,8 @@ CreateCheckPoint(int flags)
 	 * the end of the last checkpoint record, and its redo pointer must point
 	 * to itself.
 	 */
-	if ((flags & (CHECKPOINT_IS_SHUTDOWN | CHECKPOINT_FORCE)) == 0)
+	if ((flags & (CHECKPOINT_IS_SHUTDOWN | CHECKPOINT_IS_STARTUP |
+				  CHECKPOINT_FORCE)) == 0)
 	{
 		XLogRecPtr	curInsert;
 
@@ -6528,7 +6540,7 @@ CreateCheckPoint(int flags)
 	 * in subtrans.c).	During recovery, though, we mustn't do this because
 	 * StartupSUBTRANS hasn't been called yet.
 	 */
-	if (!InRecovery)
+	if (!InRecovery && (flags & CHECKPOINT_IS_STARTUP) == 0)
 		TruncateSUBTRANS(GetOldestXmin(true, false));
 
 	/* All real work is done, but log before releasing lock. */
diff --git a/src/backend/postmaster/bgwriter.c b/src/backend/postmaster/bgwriter.c
index 55dff57..2c42584 100644
--- a/src/backend/postmaster/bgwriter.c
+++ b/src/backend/postmaster/bgwriter.c
@@ -449,6 +449,13 @@ BackgroundWriterMain(void)
 			SpinLockRelease(&bgs->ckpt_lck);
 
 			/*
+			 * A startup checkpoint is a real checkpoint that's performed
+			 * while we're still in recovery.
+			 */
+			if (flags & CHECKPOINT_IS_STARTUP)
+				do_restartpoint = false;
+
+			/*
 			 * We will warn if (a) too soon since last checkpoint (whatever
 			 * caused it) and (b) somebody set the CHECKPOINT_CAUSE_XLOG flag
 			 * since the last checkpoint start.  Note in particular that this
@@ -895,10 +902,12 @@ BgWriterShmemInit(void)
  *
  * flags is a bitwise OR of the following:
  *	CHECKPOINT_IS_SHUTDOWN: checkpoint is for database shutdown.
+ *	CHECKPOINT_IS_STARTUP: checkpoint is for database startup.
  *	CHECKPOINT_IMMEDIATE: finish the checkpoint ASAP,
  *		ignoring checkpoint_completion_target parameter.
  *	CHECKPOINT_FORCE: force a checkpoint even if no XLOG activity has occured
- *		since the last one (implied by CHECKPOINT_IS_SHUTDOWN).
+ *		since the last one (implied by CHECKPOINT_IS_SHUTDOWN and
+ *		CHECKPOINT_IS_STARTUP).
  *	CHECKPOINT_WAIT: wait for completion before returning (otherwise,
  *		just signal bgwriter to do it, and return).
  *	CHECKPOINT_CAUSE_XLOG: checkpoint is requested due to xlog filling.
diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index d42e86a..b01b2ab 100644
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -204,6 +204,21 @@ mdinit(void)
 }
 
 /*
+ * In archive recovery, we rely on bgwriter to do fsyncs(), but we don't
+ * know that we do archive recovery at process startup when pendingOpsTable
+ * has already been created. Calling this function drops pendingOpsTable
+ * and causes any subsequent requests to be forwarded to bgwriter.
+ */
+void
+SetForwardFsyncRequests(void)
+{
+	/* Perform any pending ops we may have queued up */
+	if (pendingOpsTable)
+		mdsync();
+	pendingOpsTable = NULL;
+}
+
+/*
  *	mdexists() -- Does the physical file exist?
  *
  * Note: this will return true for lingering files, with pending deletions
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index f8720bb..d206b93 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -166,6 +166,7 @@ extern bool XLOG_DEBUG;
 /* These indicate the cause of a checkpoint request */
 #define CHECKPOINT_CAUSE_XLOG	0x0010	/* XLOG consumption */
 #define CHECKPOINT_CAUSE_TIME	0x0020	/* Elapsed time */
+#define CHECKPOINT_IS_STARTUP	0x0040	/* Checkpoint is for startup */
 
 /* Checkpoint statistics */
 typedef struct CheckpointStatsData
diff --git a/src/include/storage/smgr.h b/src/include/storage/smgr.h
index 7556b14..fd79d7b 100644
--- a/src/include/storage/smgr.h
+++ b/src/include/storage/smgr.h
@@ -109,6 +109,7 @@ extern void mdpreckpt(void);
 extern void mdsync(void);
 extern void mdpostckpt(void);
 
+extern void SetForwardFsyncRequests(void);
 extern void RememberFsyncRequest(RelFileNode rnode, ForkNumber forknum,
 					 BlockNumber segno);
 extern void ForgetRelationFsyncRequests(RelFileNode rnode, ForkNumber forknum);

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Simon Riggs wrote:
> On Fri, 2009-06-26 at 05:14 +0100, Simon Riggs wrote:
>> On Thu, 2009-06-25 at 20:25 -0400, Tom Lane wrote:
> 
>>> What I am thinking is that instead of the hack of clearing
>>> LocalRecoveryInProgress to allow the current process to write WAL,
>>> we should have a separate test function WALWriteAllowed() with a
>>> state variable LocalWALWriteAllowed, and the hack should set that
>>> state without playing any games with LocalRecoveryInProgress.  Then
>>> RecoveryInProgress() remains true during the end-of-recovery checkpoint
>>> and we can condition the TruncateMultiXact and TruncateSUBTRANS calls
>>> on that.  Meanwhile the various places that check RecoveryInProgress
>>> to decide if WAL writing is allowed should call WALWriteAllowed()
>>> instead.
>> No need.
> 
> Belay that. Yes, agree need for additional state, though think its more
> like EndRecoveryIsComplete(). 

Here's a patch implementing the WALWriteAllowed() idea (I'm not wedded
to the name). There's two things that trouble me with this patch:

- CreateCheckPoint() calls AdvanceXLInsertBuffer() before setting
LocalWALWriteAllowed. I don't see anything in AdvanceXLInsertBuffer()
that would fail, but it doesn't feel right. While strictly speaking it
doesn't insert new WAL records, it does write WAL page headers.

- As noted with an XXX comment in the patch, CreateCheckPoint() now
resets LocalWALWriteAllowed to false after a shutdown/end-of-recovery
checkpoint. But that's not enough to stop WAL inserts after a shutdown
checkpoint, because when RecoveryInProgress() is false, we
WALWriteAllowed() still returns true. We haven't had such a safeguard in
place before, so we can keep living without it, but now that we have a
WALWriteAllowed() macro it would be nice if it returned false when WAL
writes are no longer allowed after a shutdown checkpoint. (that would've
caught a bug in Guillaume Smet's original patch to rotate a WAL segment
at shutdown, where the xlog switch was done after shutdown checkpoint)

On whole, this is probably the right way going forward, but I'm not sure
if it'd make 8.4 more or less robust than what's in CVS now.

-- 
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 7314341..6f86961 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -1543,7 +1543,7 @@ CheckPointMultiXact(void)
 	 * SimpleLruTruncate would get confused.  It seems best not to risk
 	 * removing any data during recovery anyway, so don't truncate.
 	 */
-	if (!InRecovery)
+	if (!RecoveryInProgress())
 		TruncateMultiXact();
 
 	TRACE_POSTGRESQL_MULTIXACT_CHECKPOINT_DONE(true);
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index d6f63c7..f28bd4d 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -142,6 +142,16 @@ static bool InArchiveRecovery = false;
  */
 static bool LocalRecoveryInProgress = true;
 
+/*
+ * Am I allowed to write new WAL records? It's always allowed after recovery.
+ * End-of-recovery checkpoint sets LocalWALWriteAllowed before we exit
+ * recovery, so that it can write the checkpoint record even though
+ * RecoveryInProgress() is still true.
+ */
+#define WALWriteAllowed() (LocalWALWriteAllowed || !RecoveryInProgress())
+
+static bool LocalWALWriteAllowed = false;
+
 /* Was the last xlog file restored from archive, or local? */
 static bool restoredFromArchive = false;
 
@@ -537,7 +547,7 @@ XLogInsert(RmgrId rmid, uint8 info, XLogRecData *rdata)
 	bool		isLogSwitch = (rmid == RM_XLOG_ID && info == XLOG_SWITCH);
 
 	/* cross-check on whether we should be here or not */
-	if (RecoveryInProgress())
+	if (!WALWriteAllowed())
 		elog(FATAL, "cannot make new WAL entries during recovery");
 
 	/* info's high bits are reserved for use by me */
@@ -1846,7 +1856,7 @@ XLogFlush(XLogRecPtr record)
 	 * During REDO, we don't try to flush the WAL, but update minRecoveryPoint
 	 * instead.
 	 */
-	if (RecoveryInProgress())
+	if (!WALWriteAllowed())
 	{
 		UpdateMinRecoveryPoint(record, false);
 		return;
@@ -1949,7 +1959,7 @@ XLogFlush(XLogRecPtr record)
 	 * and so we will not force a restart for a bad LSN on a data page.
 	 */
 	if (XLByteLT(LogwrtResult.Flush, record))
-		elog(InRecovery ? WARNING : ERROR,
+		elog(RecoveryInProgress() ? WARNING : ERROR,
 		"xlog flush request %X/%X is not satisfied --- flushed only to %X/%X",
 			 record.xlogid, record.xrecoff,
 			 LogwrtResult.Flush.xlogid, LogwrtResult.Flush.xrecoff);
@@ -1977,7 +1987,7 @@ XLogBackgroundFlush(void)
 	bool		flexible = true;
 
 	/* XLOG doesn't need flushing during recovery */
-	if (RecoveryInProgress())
+	if (!WALWriteAllowed())
 		return;
 
 	/* read LogwrtResult and update local state */
@@ -5849,7 +5859,10 @@ RecoveryInProgress(void)
 		 * recovery is finished.
 		 */
 		if (!LocalRecoveryInProgress)
+		{
 			InitXLOGAccess();
+			LocalWALWriteAllowed = true;
+		}
 
 		return LocalRecoveryInProgress;
 	}
@@ -6225,7 +6238,6 @@ CreateCheckPoint(int flags)
 	uint32		_logSeg;
 	TransactionId *inCommitXids;
 	int			nInCommit;
-	bool		OldInRecovery = InRecovery;
 
 	/*
 	 * An end-of-recovery checkpoint is really a shutdown checkpoint, just
@@ -6236,33 +6248,9 @@ CreateCheckPoint(int flags)
 	else
 		shutdown = false;
 
-	/*
-	 * A startup checkpoint is created before anyone else is allowed to
-	 * write WAL. To allow us to write the checkpoint record, set
-	 * LocalRecoveryInProgress to false. This lets us write WAL, but others
-	 * are still not allowed to do so.
-	 */
-	if (flags & CHECKPOINT_END_OF_RECOVERY)
-	{
-		Assert(RecoveryInProgress());
-		LocalRecoveryInProgress = false;
-		InitXLOGAccess();
-
-		/*
-		 * Before 8.4, end-of-recovery checkpoints were always performed by
-		 * the startup process, and InRecovery was set true. InRecovery is not
-		 * normally set in bgwriter, but we set it here temporarily to avoid
-		 * confusing old code in the end-of-recovery checkpoint code path that
-		 * rely on it.
-		 */
-		InRecovery = true;
-	}
-	else
-	{
-		/* shouldn't happen */
-		if (RecoveryInProgress())
-			elog(ERROR, "can't create a checkpoint during recovery");
-	}
+	/* shouldn't happen */
+	if (RecoveryInProgress() && (flags & CHECKPOINT_END_OF_RECOVERY) == 0)
+		elog(ERROR, "can't create a checkpoint during recovery");
 
 	/*
 	 * Acquire CheckpointLock to ensure only one checkpoint happens at a time.
@@ -6305,7 +6293,6 @@ CreateCheckPoint(int flags)
 
 	/* Begin filling in the checkpoint WAL record */
 	MemSet(&checkPoint, 0, sizeof(checkPoint));
-	checkPoint.ThisTimeLineID = ThisTimeLineID;
 	checkPoint.time = (pg_time_t) time(NULL);
 
 	/*
@@ -6473,6 +6460,24 @@ CreateCheckPoint(int flags)
 	START_CRIT_SECTION();
 
 	/*
+	 * An end-of-recovery checkpoint is created before anyone is allowed to
+	 * write WAL. To allow us to write the checkpoint record, temporarily
+	 * enable LocalWALWriteAllowed.
+	 */
+	if (flags & CHECKPOINT_END_OF_RECOVERY)
+	{
+		Assert(RecoveryInProgress());
+		LocalWALWriteAllowed = true;
+		InitXLOGAccess();
+	}
+
+	/*
+	 * this needs to be done after the InitXLOGAccess() call above or
+	 * ThisTimeLineID might be uninitialized
+	 */
+	checkPoint.ThisTimeLineID = ThisTimeLineID;
+
+	/*
 	 * Now insert the checkpoint record into XLOG.
 	 */
 	rdata.data = (char *) (&checkPoint);
@@ -6488,6 +6493,19 @@ CreateCheckPoint(int flags)
 	XLogFlush(recptr);
 
 	/*
+	 * We mustn't write any new WAL after a shutdown checkpoint, or it will
+	 * be overwritten at next startup. No-one should even try, this just
+	 * allows a bit more sanity-checking. XXX: since RecoveryInProgress() is
+	 * false at that point, we'll just set LocalWriteAllowed again if anyone
+	 * calls XLogInsert(), so this is actually useless in the shutdown case.
+	 *
+	 * Don't allow WAL writes after an end-of-recovery checkpoint either. It
+	 * will be enabled again after the startup is fully completed.
+	 */
+	if (shutdown)
+		LocalWALWriteAllowed = false;
+
+	/*
 	 * We now have ProcLastRecPtr = start of actual checkpoint record, recptr
 	 * = end of actual checkpoint record.
 	 */
@@ -6560,7 +6578,7 @@ CreateCheckPoint(int flags)
 	 * in subtrans.c).	During recovery, though, we mustn't do this because
 	 * StartupSUBTRANS hasn't been called yet.
 	 */
-	if (!InRecovery)
+	if (!RecoveryInProgress())
 		TruncateSUBTRANS(GetOldestXmin(true, false));
 
 	/* All real work is done, but log before releasing lock. */
@@ -6574,9 +6592,6 @@ CreateCheckPoint(int flags)
 									 CheckpointStats.ckpt_segs_recycled);
 
 	LWLockRelease(CheckpointLock);
-
-	/* Restore old value */
-	InRecovery = OldInRecovery;
 }
 
 /*

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Re: BUG #4879: bgwriter fails to fsync the file in recovery mode