Hi

I believe I have discovered the following problem in pgsql 8.2 and HEAD,
concerning warm-standbys using WAL log shipping.

The problem is that after a crash, the master might complete incomplete
actions via rm_cleanup() - but since it won't wal-log those changes,
the slave won't know about this. This will at least prevent the creation
of any further restart points on the slave (because safe_restartpoint)
will never return true again - it it might even cause data corruption,
if subsequent wal records are interpreted wrongly by the slave because
it sees other data than the master did when it generated them.

Attached is a patch that lets RecoveryRestartPoint call all
rm_cleanup() methods and create a restart point whenever it encounters
a shutdown checkpoint in the wal (because those are generated after
recovery). This ought not cause a performance degradation, because
shutdown checkpoints will occur very infrequently.

The patch is per discussion with Simon Riggs.

I've not yet had a chance to test this patch, I only made sure
that it compiles. I'm sending this out now because I hope this
might make it into 8.2.4.

greetings, Florian Pflug
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 6c67821..93c86a1 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -5060,10 +5060,13 @@ #endif
 		 * Perform a checkpoint to update all our recovery activity to disk.
 		 *
 		 * Note that we write a shutdown checkpoint rather than an on-line
-		 * one. This is not particularly critical, but since we may be
-		 * 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.
+		 * one. A slave will always create a restart point if it sees a
+		 * shutdown checkpoint, and will call all rm_cleanup() methods before
+		 * it does so. This guarantees that any actions taken by the master
+		 * in rm_cleanup will also be carried out on the slave.
+		 * Additionally, we may be assigning a new TLI, so using a shutdow
+		 * checkpoint allows us to have the rule that TLI only changes in shutdown
+		 * checkpoints, which allows some extra error checking in xlog_redo.
 		 */
 		CreateCheckPoint(true, true);
 
@@ -5672,35 +5675,56 @@ CheckPointGuts(XLogRecPtr checkPointRedo
  * restartpoint is needed or not.
  */
 static void
-RecoveryRestartPoint(const CheckPoint *checkPoint)
+RecoveryRestartPoint(const CheckPoint *checkPoint, const bool shutdownCheckpoint)
 {
 	int			elapsed_secs;
 	int			rmid;
 
 	/*
-	 * Do nothing if the elapsed time since the last restartpoint is less than
-	 * half of checkpoint_timeout.	(We use a value less than
-	 * checkpoint_timeout so that variations in the timing of checkpoints on
-	 * the master, or speed of transmission of WAL segments to a slave, won't
-	 * make the slave skip a restartpoint once it's synced with the master.)
-	 * Checking true elapsed time keeps us from doing restartpoints too often
-	 * while rapidly scanning large amounts of WAL.
+	 * If the checkpoint we saw in the wal was a shutdown checkpoint, it might
+	 * have been written after the recovery following a crash of the master.
+	 * In that case, the master will have completed any actions that were
+	 * incomplete when it crashed *during recovery*, and these completions
+	 * are therefor *not* logged in the wal.
+	 * To prevent getting out of sync, we follow what the master did, and
+	 * call the rm_cleanup() methods. To be on the safe side, we then create
+	 * a RestartPoint, regardless of the time elapsed. Note that asking
+	 * the resource managers if they have partial state would be redundant
+	 * after calling rm_cleanup().
 	 */
-	elapsed_secs = time(NULL) - ControlFile->time;
-	if (elapsed_secs < CheckPointTimeout / 2)
-		return;
+	if (shutdownCheckpoint) {
+		for (rmid = 0; rmid <= RM_MAX_ID; rmid++)
+		{
+			if (RmgrTable[rmid].rm_cleanup != NULL)
+				RmgrTable[rmid].rm_cleanup();
+		}
+	}
+	else {
+		/*
+		 * Do nothing if the elapsed time since the last restartpoint is less than
+		 * half of checkpoint_timeout.	(We use a value less than
+		 * checkpoint_timeout so that variations in the timing of checkpoints on
+		 * the master, or speed of transmission of WAL segments to a slave, won't
+		 * make the slave skip a restartpoint once it's synced with the master.)
+		 * Checking true elapsed time keeps us from doing restartpoints too often
+		 * while rapidly scanning large amounts of WAL.
+		 */
+		elapsed_secs = time(NULL) - ControlFile->time;
+		if (elapsed_secs < CheckPointTimeout / 2)
+			return;
 
-	/*
-	 * Is it safe to checkpoint?  We must ask each of the resource managers
-	 * whether they have any partial state information that might prevent a
-	 * correct restart from this point.  If so, we skip this opportunity, but
-	 * return at the next checkpoint record for another try.
-	 */
-	for (rmid = 0; rmid <= RM_MAX_ID; rmid++)
-	{
-		if (RmgrTable[rmid].rm_safe_restartpoint != NULL)
-			if (!(RmgrTable[rmid].rm_safe_restartpoint()))
-				return;
+		/*
+		 * Is it safe to checkpoint?  We must ask each of the resource managers
+		 * whether they have any partial state information that might prevent a
+		 * correct restart from this point.  If so, we skip this opportunity, but
+		 * return at the next checkpoint record for another try.
+		 */
+		for (rmid = 0; rmid <= RM_MAX_ID; rmid++)
+		{
+			if (RmgrTable[rmid].rm_safe_restartpoint != NULL)
+				if (!(RmgrTable[rmid].rm_safe_restartpoint()))
+					return;
+		}
 	}
 
 	/*
@@ -5835,7 +5859,7 @@ xlog_redo(XLogRecPtr lsn, XLogRecord *re
 			ThisTimeLineID = checkPoint.ThisTimeLineID;
 		}
 
-		RecoveryRestartPoint(&checkPoint);
+		RecoveryRestartPoint(&checkPoint, true);
 	}
 	else if (info == XLOG_CHECKPOINT_ONLINE)
 	{
@@ -5864,7 +5888,7 @@ xlog_redo(XLogRecPtr lsn, XLogRecord *re
 					(errmsg("unexpected timeline ID %u (should be %u) in checkpoint record",
 							checkPoint.ThisTimeLineID, ThisTimeLineID)));
 
-		RecoveryRestartPoint(&checkPoint);
+		RecoveryRestartPoint(&checkPoint, false);
 	}
 	else if (info == XLOG_SWITCH)
 	{
---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster

Reply via email to