From: Anastasia Lubennikova <a.lubennikova@postgrespro.ru>
To: Michael Paquier <michael@paquier.xyz>
Cc: Michael Banck <michael.banck@credativ.de>
Cc: PostgreSQL Development <pgsql-hackers@postgresql.org>
Subject: Re: [patch] Fix checksum verification in base backups for zero page headers
Date: Thu, 22 Oct 2020 00:47:03 +0300
Message-ID: <69b38451-e91f-95fc-c3ce-06eb778d36e0@postgrespro.ru> (raw)
In-Reply-To: <20201020062432.GA30362@paquier.xyz>
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>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: a.lubennikova@postgrespro.ru, michael@paquier.xyz, michael.banck@credativ.de
Subject: Re: [patch] Fix checksum verification in base backups for zero page headers
In-Reply-To: <69b38451-e91f-95fc-c3ce-06eb778d36e0@postgrespro.ru>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox