(.. 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