Re: [patch] Fix checksum verification in base backups for zero page headers

From: Anastasia Lubennikova <a(dot)lubennikova(at)postgrespro(dot)ru>
To: Michael Paquier <michael(at)paquier(dot)xyz>, Michael Banck <michael(dot)banck(at)credativ(dot)de>
Cc: PostgreSQL Development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [patch] Fix checksum verification in base backups for zero page headers
Date: 2020-10-16 15:02:52
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 07.10.2020 11:18, Michael Paquier wrote:
> On Fri, Sep 25, 2020 at 08:53:27AM +0200, Michael Banck wrote:
>> Oh right, I've fixed up the commit message in the attached V4.
> Not much a fan of what's proposed here, for a couple of reasons:
> - If the page is not new, we should check if the header is sane or
> not.
> - It may be better to actually count checksum failures if these are
> repeatable in a given block, and report them.
> - The code would be more simple with a "continue" added for a block
> detected as new or with a LSN newer than the backup start.
> - The new error messages are confusing, and I think that these are
> hard to translate in a meaningful way.
I am interested in moving this patch forward, so here is the updated v5.
This version uses PageIsVerified. All error messages are left unchanged.
> So I think that we should try to use PageIsVerified() directly in the
> code path of basebackup.c, and this requires a couple of changes to
> make the routine more extensible:
> - Addition of a dboid argument for pgstat_report_checksum_failure(),
> whose call needs to be changed to use the *_in_db() flavor.

In the current patch, PageIsVerifed called from pgbasebackup simply
doesn't report failures to pgstat.
Because in basebackup.c we already report checksum failures separately.
Once per file.
        pgstat_report_checksum_failures_in_db(dboid, checksum_failures);

Should we change this too? I don't see any difference.

> - Addition of an option to decide if a log should be generated or
> not.
> - Addition of an option to control if a checksum failure should be
> reported to pgstat or not, even if this is actually linked to the
> previous point, I have seen cases where being able to control both
> separately would be helpful, particularly the log part.
> For the last two ones, I would use an extensible argument based on
> bits32 with a set of flags that the caller can set at will.

Anastasia Lubennikova
Postgres Professional:
The Russian Postgres Company

Attachment Content-Type Size
0001-Fix-checksum-verification-in-base-backups-for-zero-p.V5.patch text/x-patch 7.0 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Bruce Momjian 2020-10-16 15:04:49 Re: Feature improvement: can we add queryId for pg_catalog.pg_stat_activity view?
Previous Message Alvaro Herrera 2020-10-16 14:45:41 Re: partition routing layering in nodeModifyTable.c