Re: corruption of WAL page header is never reported

(.. sorry for the chained-mails)

At Thu, 02 Sep 2021 13:31:31 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> Sorry, please let me add something.
> 
> At Thu, 02 Sep 2021 13:17:16 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> > At Thu, 2 Sep 2021 12:19:25 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in 
> > > 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?
> > 
> > I don't think it is good choice to conflate read-failure and header
> > validation failure from the view of modularity.
> 
> In other words, XLogReaderValidatePageHeader is foreign for
> XLogPageRead and the function indeuces the need of extra care for
> errormsg_buf that is not relevant to the elog-capable module.

However, I can agree that the error handling code can be moved further
later.  Like this,

> 	 * 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;
+   if (... && XLogReaderValidatePageHeader())
+       goto page_header_is_invalid;
...
>   return readlen;
>
+ page_header_is_invalid:
+	/*
+	 * in this case we consume this error right now then retry immediately,
+	 * the message is already translated
+	 */
+	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';
> 
> next_record_is_invalid:
> 	lastSourceFailed = true;

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center