Re: [Patch] Base backups and random or zero pageheaders (was: Online verification of checksums)

From: Michael Banck <michael(dot)banck(at)credativ(dot)de>
To: Andres Freund <andres(at)anarazel(dot)de>
Cc: Stephen Frost <sfrost(at)snowman(dot)net>, Michael Paquier <michael(at)paquier(dot)xyz>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Robert Haas <robertmhaas(at)gmail(dot)com>, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>
Subject: Re: [Patch] Base backups and random or zero pageheaders (was: Online verification of checksums)
Date: 2019-03-26 18:23:19
Message-ID: 1553624599.4884.24.camel@credativ.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi,

Am Dienstag, den 26.03.2019, 10:30 -0700 schrieb Andres Freund:
> On 2019-03-26 18:22:55 +0100, Michael Banck wrote:
> > Am Dienstag, den 19.03.2019, 13:00 -0700 schrieb Andres Freund:
> > > CREATE TABLE corruptme AS SELECT g.i::text AS data FROM generate_series(1, 1000000) g(i);
> > > SELECT pg_relation_size('corruptme');
> > > postgres[22890][1]=# SELECT current_setting('data_directory') || '/' || pg_relation_filepath('corruptme');
> > > ┌─────────────────────────────────────┐
> > > │ ?column? │
> > > ├─────────────────────────────────────┤
> > > │ /srv/dev/pgdev-dev/base/13390/16384 │
> > > └─────────────────────────────────────┘
> > > (1 row)
> > > dd if=/dev/urandom of=/srv/dev/pgdev-dev/base/13390/16384 bs=8192 count=1 conv=notrunc
> > >
> > > Try a basebackup and see how many times it'll detect the corrupt
> > > data. In the vast majority of cases you're going to see checksum
> > > failures when reading the data for normal operation, but not when using
> > > basebackup (or this new tool).
> > >
> > > At the very very least this would need to do
> > >
> > > a) checks that the page is all zeroes if PageIsNew() (like
> > > PageIsVerified() does for the backend). That avoids missing cases
> > > where corruption just zeroed out the header, but not the whole page.
> > > b) Check that pd_lsn is between startlsn and the insertion pointer. That
> > > avoids accepting just about all random data.
> > >
> > > And that'd *still* be less strenuous than what normal backends
> > > check. And that's already not great (due to not noticing zeroed out
> > > data).
> >
> > I've done the above in the attached patch now. Well, literally like an
> > hour ago, then went jogging and came back to see you outlined about
> > fixing this differently in a separate thread. Still might be helpful for
> > the TAP test changes at least.
>
> Sorry, I just hadn't seen much movement on this, and I'm a bit concerned
> about such a critical issue not being addressed.

Sure, I was working on this a bit on and off for a few days but I had
random corruption issues which I finally tracked down earlier to reusing
"for (i=0 [...]" via copy&paste, d'oh. That's why I renamed the `i'
variable to `page_in_buf' cause it's a pretty long loop so should have a
useful variable name IMO.

> > /*
> > - * Only check pages which have not been modified since the
> > - * start of the base backup. Otherwise, they might have been
> > - * written only halfway and the checksum would not be valid.
> > - * However, replaying WAL would reinstate the correct page in
> > - * this case. We also skip completely new pages, since they
> > - * don't have a checksum yet.
> > + * We skip completely new pages after checking they are
> > + * all-zero, since they don't have a checksum yet.
> > */
> > - if (!PageIsNew(page) && PageGetLSN(page) < startptr)
> > + if (PageIsNew(page))
> > {
> > - checksum = pg_checksum_page((char *) page, blkno + segmentno * RELSEG_SIZE);
> > - phdr = (PageHeader) page;
> > - if (phdr->pd_checksum != checksum)
> > + all_zeroes = true;
> > + pagebytes = (size_t *) page;
> > + for (int i = 0; i < (BLCKSZ / sizeof(size_t)); i++)
>
> Can we please abstract the zeroeness check into a separate function to
> be used both by PageIsVerified() and this?

Ok, done so as PageIsZero further down in bufpage.c.

> > + if (!all_zeroes)
> > + {
> > + /*
> > + * pd_upper is zero, but the page is not all zero. We
> > + * cannot run pg_checksum_page() on the page as it
> > + * would throw an assertion failure. Consider this a
> > + * checksum failure.
> > + */
>
> I don't think the assertion failure is the relevant bit here, it's htat
> the page is corrupted, no?

Well, relevant in the sense that the reader might wonder why we don't
just call pg_checksum_page() and have a consistent error message with
the other codepath.

We could maybe run pg_checksum_block() on it and reverse the rest of the
permutations from pg_checksum_page() but that might be overly
complicated for little gain.

> > + /*
> > + * Only check pages which have not been modified since the
> > + * start of the base backup. Otherwise, they might have been
> > + * written only halfway and the checksum would not be valid.
> > + * However, replaying WAL would reinstate the correct page in
> > + * this case. If the page LSN is larger than the current redo
> > + * pointer then we assume a bogus LSN due to random page header
> > + * corruption and do verify the checksum.
> > + */
> > + if (PageGetLSN(page) < startptr || PageGetLSN(page) > GetRedoRecPtr())
>
> I don't think GetRedoRecPtr() is the right check? Wouldn't it need to be
> GetInsertRecPtr()?

Oh, right.

I also fixed a bug in the TAP tests, the $random_data string wasn't
properly set.

New patch attached.

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

Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz

Attachment Content-Type Size
pg_basebackup_random_or_zero_pageheader_V2.patch text/x-patch 11.3 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2019-03-26 18:25:52 Re: pgsql: Get rid of backtracking in jsonpath_scan.l
Previous Message Alvaro Herrera 2019-03-26 18:00:10 Re: pgsql: Get rid of backtracking in jsonpath_scan.l