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 |
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 |