|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|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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
> - 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.
Should we change this too? I don't see any difference.
> - Addition of an option to decide if a log should be generated or
> - 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.
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
|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|