Re: [PATCH] BUG FIX: inconsistent page found in BRIN_REGULAR_PAGE

At Wed, 3 Aug 2022 02:37:30 -0400, 王海洋 <wanghaiyang(dot)001(at)bytedance(dot)com> wrote in
> Hi hackers,
>
> I found that when wal_consistency_checking = brin is set, it may cause redo
> abort, all the standby-nodes lost, and the primary node can not be restart.
>
> This bug exists in all versions of PostgreSQL.
>
> The operation steps are as follows:

Nice reproducer!

> I analyzed the reasons as follows:
>
> 1. When the revmap needs to be extended by brinRevmapExtend,
> we may set BRIN_EVACUATE_PAGE flag on a REGULAR_PAGE to prevent
> other concurrent backends from adding more BrinTuple to that page
> in brin_start_evacuating_page.
>
> 2. But, during redo-process, it is not needed to set BRIN_EVACUATE_PAGE
> flag on that REGULAR_PAGE after removing the old BrinTuple in
> brin_xlog_update, since no one will add BrinTuple to that Page at
> this time.
>
> 3. As a result, this will cause a FATAL message to be thrown in
> CheckXLogConsistency after redo, due to inconsistency checking of
> the BRIN_EVACUATE_PAGE flag, finally cause redo to abort.
>
> 4. Therefore, the BRIN_EVACUATE_PAGE flag should be cleared before
> CheckXLogConsistency.

+1 for what the patch does. The flag is actually non-logged,
primary-only flag.

The coment looks too verbose. It looks as if saying we evading some
flaw by masking the flag. The inconsistency is caused by that the flag
is changed unlogged and not operated via wal-redo. So, I think it can
be shortened as the following example. (It is taken from btree_mask())

> BRIN_EVACUATE_PAGE is an unlogged bit, which is never set during
> recovery. See brin_start_evacuating_page() for details.

I think we need the corresponding comment that explains we don't need
to wal-log the bit in the function, too.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center