(.. sorry for the chained-mails) At Thu, 02 Sep 2021 13:31:31 +0900 (JST), Kyotaro Horiguchi <horikyota....@gmail.com> wrote in > Sorry, please let me add something. > > At Thu, 02 Sep 2021 13:17:16 +0900 (JST), Kyotaro Horiguchi > <horikyota....@gmail.com> wrote in > > At Thu, 2 Sep 2021 12:19:25 +0900, Fujii Masao > > <masao.fu...@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