Subject: Re: [patch] Fix checksum verification in base backups for zero page
 headers
To: Michael Banck <michael.banck@credativ.de>
Cc: PostgreSQL Development <pgsql-hackers@postgresql.org>
References: <2439c2d6cee13d611e23ba651e5eb4ab0469bf99.camel@credativ.de>
 <d041090c-ddf7-10d1-7d50-2a4ed03c53dd@postgrespro.ru>
 <4b1c16216dcb60e1dad96142d25fd06fb77485bf.camel@credativ.de>
 <6713b5e9d98c7db1608b43cd57b10be4a08dca67.camel@credativ.de>
From: Anastasia Lubennikova <a.lubennikova@postgrespro.ru>
Message-ID: <9489c221-e2ef-e7ad-d921-5ccad71b14de@postgrespro.ru>
Date: Thu, 24 Sep 2020 17:24:27 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <6713b5e9d98c7db1608b43cd57b10be4a08dca67.camel@credativ.de>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Precedence: bulk


On 22.09.2020 17:30, Michael Banck wrote:
> Hi,
>
> Am Dienstag, den 22.09.2020, 16:26 +0200 schrieb Michael Banck:
>> Am Mittwoch, den 02.09.2020, 16:50 +0300 schrieb Anastasia Lubennikova:
>>> I've looked through the previous discussion. As far as I got it, most of
>>> the controversy was about online checksums improvements.
>>>
>>> The warning about pd_upper inconsistency that you've added is a good
>>> addition. The patch is a bit messy, though, because a huge code block
>>> was shifted.
>>>
>>> Will it be different, if you just leave
>>>       "if (!PageIsNew(page) && PageGetLSN(page) < startptr)"
>>> block as it was, and add
>>>       "else if (PageIsNew(page) && !PageIsZero(page))" ?
>> Thanks, that does indeed look better as a patch and I think it's fine
>> as-is for the code as well, I've attached a v2.
> Sorry, forgot to add you as reviewer in the proposed commit message,
> I've fixed that up now in V3.
>
>
> Michael
>
Great. This version looks good to me.
Thank you for answering my questions, I agree, that we can work on them 
in separate threads.

So I mark this one as ReadyForCommitter.

The only minor problem is a typo (?) in the proposed commit message.
"If a page is all zero, consider that a checksum failure." It should be 
"If a page is NOT all zero...".

-- 
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company