Re: corruption of WAL page header is never reported

On 2021/07/19 18:52, Yugo NAGATA wrote:
> Well, I think my first patch was not wrong. The difference with the latest
> patch is just whether we perform the additional check when we are not in
> standby mode or not. The behavior is basically the same although which function
> detects and prints the page-header error in cases of crash recovery is different.

Yes, so which patch do you think is better? I like your version
because there seems no reason why XLogPageRead() should skip
XLogReaderValidatePageHeader() when not in standby mode.

Also I'm tempted to move ereport() and reset of errmsg_buf to
under next_record_is_invalid as follows. That is, in standby mode
whenever we find an invalid record and retry reading WAL page
in XLogPageRead(), we report the error message and reset it.
For now in XLogPageRead(), only XLogReaderValidatePageHeader()
sets errmsg_buf, but in the future other code or function doing that
may be added. For that case, the following change seems more elegant.
Thought?

	 * shouldn't be a big deal from a performance point of view.
  	 */
  	if (!XLogReaderValidatePageHeader(xlogreader, targetPagePtr, readBuf))
-	{
-		/* reset any error XLogReaderValidatePageHeader() might have set */
-		xlogreader->errormsg_buf[0] = '\0';
  		goto next_record_is_invalid;
-	}
  
  	return readLen;
  
@@ -12517,7 +12513,17 @@ next_record_is_invalid:
  
  	/* In standby-mode, keep trying */
  	if (StandbyMode)
+	{
+		if (xlogreader->errormsg_buf[0])
+		{
+			ereport(emode_for_corrupt_record(emode, EndRecPtr),
+					(errmsg_internal("%s", xlogreader->errormsg_buf)));
+
+			/* reset any error XLogReaderValidatePageHeader() might have set */
+			xlogreader->errormsg_buf[0] = '\0';
+		}
  		goto retry;
+	}
  	else
  		return -1;
  }

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION