Re: corruption of WAL page header is never reported - Mailing list pgsql-hackers

From Kyotaro Horiguchi
Subject Re: corruption of WAL page header is never reported
Date
Msg-id 20210902.133958.2257805981735903994.horikyota.ntt@gmail.com
Whole thread Raw
In response to Re: corruption of WAL page header is never reported  (Kyotaro Horiguchi <horikyota.ntt@gmail.com>)
List pgsql-hackers
(.. 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



pgsql-hackers by date:

Previous
From: Sadhuprasad Patro
Date:
Subject: Re: Support reset of Shared objects statistics in "pg_stat_reset" function
Next
From: Fujii Masao
Date:
Subject: Re: Support reset of Shared objects statistics in "pg_stat_reset" function