Re: [PATCH] Verify Checksums during Basebackups

From: Michael Banck <michael(dot)banck(at)credativ(dot)de>
To: pgsql-hackers(at)lists(dot)postgresql(dot)org
Cc: David Steele <david(at)pgmasters(dot)net>, Stephen Frost <sfrost(at)snowman(dot)net>, Magnus Hagander <magnus(at)hagander(dot)net>, Robert Haas <robertmhaas(at)gmail(dot)com>
Subject: Re: [PATCH] Verify Checksums during Basebackups
Date: 2018-03-09 21:35:33
Message-ID: 1520631333.22202.42.camel@credativ.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi,

Am Mittwoch, den 28.02.2018, 19:08 +0100 schrieb Michael Banck:
> some installations have data which is only rarerly read, and if they are
> so large that dumps are not routinely taken, data corruption would only
> be detected with some large delay even with checksums enabled.
>
> The attached small patch verifies checksums (in case they are enabled)
> during a basebackup. The rationale is that we are reading every block in
> this case anyway, so this is a good opportunity to check them as well.
> Other and complementary ways of checking the checksums are possible of
> course, like the offline checking tool that Magnus just submitted.

I've attached a second version of this patch. Changes are:

1. I've included some code from Magnus' patch, notably the way the
segment numbers are determined and the skipfile() function, along with
the array of files to skip.

2. I am now checking the LSN in the pageheader and compare it against
the LSN of the basebackup start, so that no checksums are verified for
pages changed after basebackup start. I am not sure whether this
addresses all concerns by Stephen and David, as I am not re-reading the
page on a checksum mismatch as they are doing in pgbackrest.

3. pg_basebackup now exits with 1 if a checksum mismatch occured, but it
keeps the data around.

4. I added an Assert() that the TAR_SEND_SIZE is a multiple of BLCKSZ.
AFAICT we support block sizes of 1, 2, 4, 8, 16 and 32 KB, while
TAR_SEND_SIZE is set to 32 KB, so this should be fine unless somebody
mucks around with BLCKSZ manually, in which case the Assert should fire.
I compiled --with-blocksize=32 and checked that this still works as
intended.

5. I also check that the buffer we read is a multiple of BLCKSZ. If that
is not the case I emit a WARNING that the checksum cannot be checked and
pg_basebackup will exit with 1 as well.

This is how it looks like right now from pg_basebackup's POV:

postgres(at)fock:~$ initdb -k --pgdata=data1 1> /dev/null

WARNING: enabling "trust" authentication for local connections
You can change this by editing pg_hba.conf or using the option -A, or
--auth-local and --auth-host, the next time you run initdb.
postgres(at)fock:~$ pg_ctl --pgdata=data1 --log=pg1.log start
waiting for server to start.... done
server started
postgres(at)fock:~$ psql -h /tmp -c "SELECT pg_relation_filepath('pg_class')"
pg_relation_filepath
----------------------
base/12367/1259
(1 row)

postgres(at)fock:~$ echo -n "Bang!" | dd conv=notrunc oflag=seek_bytes seek=4000 bs=9 count=1 of=data1/base/12367/1259
0+1 records in
0+1 records out
5 bytes copied, 3.7487e-05 s, 133 kB/s
postgres(at)fock:~$ pg_basebackup --pgdata=data2 -h /tmp
WARNING: checksum mismatch in file "./base/12367/1259", segment 0, block 0: expected CC05, found CA4D
pg_basebackup: checksum error occured
postgres(at)fock:~$ echo $?
1
postgres(at)fock:~$ ls data2
backup_label pg_dynshmem pg_multixact pg_snapshots pg_tblspc pg_xact
base pg_hba.conf pg_notify pg_stat pg_twophase postgresql.auto.conf
global pg_ident.conf pg_replslot pg_stat_tmp PG_VERSION postgresql.conf
pg_commit_ts pg_logical pg_serial pg_subtrans pg_wal
postgres(at)fock:~$

Possibly open questions:

1. I have not so far changed the replication protocol to make verifying
checksums optional. I can go about that next if the consensus is that we
need such an option (and cannot just check it everytime)?

2. The isolation tester test uses dd (similar to the above), is that
allowed, or do I have to come up with some internal Perl thing that also
works on Windows?

3. I am using basename() to get the filename, I haven't seen that used a
lot in the codebase (nor did I find an obvious internal implementation),
is that fine?

Cheers,

Michael

--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael(dot)banck(at)credativ(dot)de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer

Attachment Content-Type Size
basebackup-verify-checksum-V2.patch text/x-patch 9.0 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Peter Eisentraut 2018-03-09 21:42:30 Re: PQHost() undefined behavior if connecting string contains both host and hostaddr types
Previous Message Andres Freund 2018-03-09 21:08:36 Re: JIT compiling with LLVM v11