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

At Wed, 3 Aug 2022 02:37:30 -0400, 王海洋 <wanghaiyang.001@bytedance.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

I'd like to explain a bit further.

At Thu, 04 Aug 2022 16:55:35 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> > BRIN_EVACUATE_PAGE is an unlogged bit, which is never set during
> > recovery.  See brin_start_evacuating_page() for details.

That being said, I think that some bug could unexpectedly set the flag
during recovery. I sought for clean way to check that only while not
in evacuation, but I found none..

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On 2022-Aug-04, 王海洋 wrote:

> Thanks, I agree. So my new modified patch is attached.

I propose the attached.  This also adds a new TAP test that runs BRIN
through a revmap extension and through wal_consistency_checking.  This
increases coverage, as currently there's none for brin_evacuate_page.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Every machine is a smoke machine if you operate it wrong enough."
https://twitter.com/libseybieda/status/1541673325781196801

Hi Alvaro!

Is there any special reason to read WAL records until the last inserted 
record?

+my $end_lsn = $whiskey->lsn('insert');
+
+my ($ret, $out, $err) = $whiskey->psql(
+    'postgres', qq{
+    select count(*) from pg_get_wal_records_info('$start_lsn', '$end_lsn')
+    where resource_manager = 'BRIN' AND
+    record_type ILIKE '%revmap%'
+    });
+cmp_ok($out, '>=', 1);

It seems that in some rare situations on slower machines this test can 
fail. If
any background process inserts a WAL record before lsn('insert') and 
this record
isn't flushed before pg_get_wal_records_info('$start_lsn', '$end_lsn'),
pg_get_wal_records_info('$start_lsn', '$end_lsn') ends with ERROR 
"cannot accept
future end LSN" as it works only if record with end LSN is inserted.

I attached two patches with two ways of fixing this minor issue.

Best regards,
Karina Litskevich
Postgres Professional: http://postgrespro.com/

Hello,

On 2022-Dec-23, Karina Litskevich wrote:

> Is there any special reason to read WAL records until the last inserted
> record?

Hmm, no, there's no good reason to do it that way.

> It seems that in some rare situations on slower machines this test can fail.
> If any background process inserts a WAL record before lsn('insert')
> and this record isn't flushed before
> pg_get_wal_records_info('$start_lsn', '$end_lsn'),
> pg_get_wal_records_info('$start_lsn', '$end_lsn') ends with ERROR
> "cannot accept future end LSN" as it works only if record with end LSN
> is inserted.

Hmm.  I've never seen that, but it sounds plausible.

> I attached two patches with two ways of fixing this minor issue.

> -my $end_lsn = $whiskey->lsn('insert');
> +my $end_lsn = $whiskey->lsn('flush');

Thank you, I used this one.

Regards

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

Evelyn,

I ran the query that runs when you click on Check Invoices  that returns the invTotal and TotDetailAmt and here's what came up. As you will see, the invTotal amounts from your list don't match the production data and most of the amounts in TotDetailAmt don't match production data. Where did your list come from? Is this an old list from the other day?

I have turned on the invTotal field in both invoice forms so you can compare the Total Invoice amount that is calculated from the line items with the invTotal amount that is saved to the database. This will allow you to see what is being saved at the time of creating the invoice.

Hello,

On 2022-Dec-23, Karina Litskevich wrote:

Is there any special reason to read WAL records until the last inserted
record?

Hmm, no, there's no good reason to do it that way.

It seems that in some rare situations on slower machines this test can fail.
If any background process inserts a WAL record before lsn('insert')
and this record isn't flushed before
pg_get_wal_records_info('$start_lsn', '$end_lsn'),
pg_get_wal_records_info('$start_lsn', '$end_lsn') ends with ERROR
"cannot accept future end LSN" as it works only if record with end LSN
is inserted.

Hmm.  I've never seen that, but it sounds plausible.

I attached two patches with two ways of fixing this minor issue.

-my $end_lsn = $whiskey->lsn('insert');
+my $end_lsn = $whiskey->lsn('flush');

Thank you, I used this one.

Regards