Re: corruption of WAL page header is never reported

On 2021/09/06 3:11, Alvaro Herrera wrote:
> On 2021-Sep-03, Kyotaro Horiguchi wrote:
>
>> diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
>> index 24165ab03e..b621ad6b0f 100644
>> --- a/src/backend/access/transam/xlog.c
>> +++ b/src/backend/access/transam/xlog.c
>> @@ -12496,9 +12496,21 @@ retry:
>> *
>> * Validating the page header is cheap enough that doing it twice
>> * shouldn't be a big deal from a performance point of view.
>> + *
>> + * Don't call XLogReaderValidatePageHeader here while not in standby mode
>> + * so that this function won't return with a valid errmsg_buf.
>> */
>> - if (!XLogReaderValidatePageHeader(xlogreader, targetPagePtr, readBuf))
>> + if (StandbyMode &&
>> + !XLogReaderValidatePageHeader(xlogreader, targetPagePtr, readBuf))
>
> OK, but I don't understand why we have a comment that says (referring to
> non-standby mode) "doing it twice shouldn't be a big deal", followed by
> "Don't do it twice while not in standby mode" -- that seems quite
> contradictory. I think the new comment should overwrite the previous
> one, something like this:
>
> - * Validating the page header is cheap enough that doing it twice
> - * shouldn't be a big deal from a performance point of view.
> + *
> + * We do this in standby mode only,
> + * so that this function won't return with a valid errmsg_buf.

Even if we do this while NOT in standby mode, ISTM that this function doesn't
return with a valid errmsg_buf because it's reset. So probably the comment
should be updated as follows?

-------------------------
We don't do this while not in standby mode because we don't need to retry
immediately if the page header is not valid. Instead, XLogReadRecord() is
responsible to check the page header.
-------------------------

Regards,

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