Subject: Re: [patch] Fix checksum verification in base backups for zero page
 headers
To: Michael Paquier <michael@paquier.xyz>
Cc: Michael Banck <michael.banck@credativ.de>,
 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>
 <9489c221-e2ef-e7ad-d921-5ccad71b14de@postgrespro.ru>
 <423deac52479a40624e775e6d2ab41c4e9f5b19f.camel@credativ.de>
 <20201007081852.GC30037@paquier.xyz>
 <608f3476-0598-2514-2c03-e05c7d2b0cbd@postgrespro.ru>
 <20201020062432.GA30362@paquier.xyz>
From: Anastasia Lubennikova <a.lubennikova@postgrespro.ru>
Message-ID: <69b38451-e91f-95fc-c3ce-06eb778d36e0@postgrespro.ru>
Date: Thu, 22 Oct 2020 00:47:03 +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: <20201020062432.GA30362@paquier.xyz>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Precedence: bulk

On 20.10.2020 09:24, Michael Paquier wrote:
> On Fri, Oct 16, 2020 at 06:02:52PM +0300, Anastasia Lubennikova wrote:
>> In the current patch, PageIsVerifed called from pgbasebackup simply doesn't
>> Should we change this too? I don't see any difference.
> I considered that, but now that does not seem worth bothering here.
>
>> Done.
> Thanks for the updated patch.  I have played with dd and some fake
> files to check the validity of the zero-case (as long as pd_lsn does
> not go wild).  Here are some comments, with an updated patch attached:
> - Added a PageIsVerifiedExtended to make this patch back-patchable,
> with the original routine keeping its original shape.
Thank you. I always forget about this. Do we have any checklist for such 
changes, that patch authors and reviewers can use?
> - I did not like much to show the WARNING from PageIsVerified() for
> the report, and we'd lose some context related to the base backups.
> The important part is to know which blocks from which files are found
> as problematic.
> - Switched the checks to have PageIsVerified() placed first in the
> hierarchy, so as we do all the basic validity checks before a look at
> the LSN.  This is not really change things logically.
> - The meaning of block_retry is also the opposite of what it should be
> in the original code?  IMO, the flag should be set to true if we still
> are fine to retry a block, and set it to false once the one-time retry
> has failed.

Agree.

> - The error strings are not really consistent with the project style
> in this area.  These are usually not spawned across multiple lines to
> ease searches with grep or such.
Same question as above. I don't see this info about formatting in the 
error message style guide in documentation. Is it mentioned somewhere else?
> Anastasia, Michael B, does that look OK to you?
The final patch looks good to me.
> NB: This patch fixes only one problem, the zero-page case, as it was
> the intention of Michael B to split this part into its own thread.  We
> still have, of course, a second problem when it comes to a random LSN
> put into the page header which could mask an actual checksum failure
> so this only takes care of half the issues.  Having a correct LSN
> approximation is a tricky problem IMO, but we could improve things by
> having some delta with an extra WAL page on top of GetInsertRecPtr().
> And this function cannot be used for base backups taken from
> standbys.

We can also read such pages via shared buffers to be 100% sure.

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