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