Thread: TAP tests for pg_verify_checksums
Hi all, The topic of $subject has been discussed a bit times, resulting in a couple of patches on the way: https://www.postgresql.org/message-id/20180830200258.GG15446@paquier.xyz https://www.postgresql.org/message-id/CABUevEzEKrwPeGK2o-OV4z2MjfT6Cu8cLfA-v1S1j4z8x7WJjg@mail.gmail.com However nothing has actually happened. Based on the previous feedback, attached is an updated patch to do the actual job. Magnus Hagander and Michael Banck need to be credited as authors, as this patch is roughly a merge of what they proposed. Thoughts? -- Michael
Attachment
Hi, On Fri, Oct 05, 2018 at 10:26:45AM +0900, Michael Paquier wrote: > The topic of $subject has been discussed a bit times, resulting in a > couple of patches on the way: > https://www.postgresql.org/message-id/20180830200258.GG15446@paquier.xyz > https://www.postgresql.org/message-id/CABUevEzEKrwPeGK2o-OV4z2MjfT6Cu8cLfA-v1S1j4z8x7WJjg@mail.gmail.com > > However nothing has actually happened. Based on the previous feedback, > attached is an updated patch to do the actual job. Thanks! It's too late for v11 though at this point I guess? I think it would be easy to also test the -r command-line option, as we already create a table. Something like |my $relfilenode_corrupted = $node->safe_psql('postgres', | "SELECT relfilenode FROM pg_class WHERE relname = 'corrupt1'"); [...] |# Checksums pass solely on that table |command_ok(['pg_verify_checksums', '-D', $pgdata, '-r', $relfilenode_corrupted], | "checksum checks for table relfilenode done and passing"); While making sure the $node->stop is between the two. One nitpick: > diff --git a/src/bin/pg_verify_checksums/t/002_actions.pl b/src/bin/pg_verify_checksums/t/002_actions.pl > new file mode 100644 > index 0000000000..7423595181 > --- /dev/null > +++ b/src/bin/pg_verify_checksums/t/002_actions.pl [...] > +# Control file should know that checksums are disabled. > +command_like(['pg_controldata', $pgdata], > + qr/Data page checksum version:.*1/, > + 'checksums enabled in control file'); That comment should read 'that checksums are enabled', right? Otherwise, LGTM and I've tested it without finding any problems. Michael -- Michael Banck Projektleiter / Senior Berater Tel.: +49 2166 9901-171 Fax: +49 2166 9901-100 Email: michael.banck@credativ.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
On Fri, Oct 05, 2018 at 01:38:05PM +0200, Michael Banck wrote: > It's too late for v11 though at this point I guess? Unfortunately yes. > I think it would be easy to also test the -r command-line option, as we > already create a table. Good idea. Let's add this test. > That comment should read 'that checksums are enabled', right? Indeed. Fixed. > Otherwise, LGTM and I've tested it without finding any problems. What do you think about the updated version attached? -- Michael
Attachment
On 06/10/2018 13:46, Michael Paquier wrote: > On Fri, Oct 05, 2018 at 01:38:05PM +0200, Michael Banck wrote: >> It's too late for v11 though at this point I guess? > > Unfortunately yes. > >> I think it would be easy to also test the -r command-line option, as we >> already create a table. > > Good idea. Let's add this test. > >> That comment should read 'that checksums are enabled', right? > > Indeed. Fixed. > >> Otherwise, LGTM and I've tested it without finding any problems. > > What do you think about the updated version attached? Looks pretty good to me. Let's make sure the test names are useful: +# Checks cannot happen for an online cluster +$node->start; +command_fails(['pg_verify_checksums', '-D', $pgdata], + "checksum checks not done"); The test name should be something like "fails with online cluster". I would also like to see a test that runs against a cluster without checksums enabled. Other than that, it appears to cover everything. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi, Am Dienstag, den 09.10.2018, 16:54 +0200 schrieb Peter Eisentraut: > On 06/10/2018 13:46, Michael Paquier wrote: > > What do you think about the updated version attached? > > +# Time to create a corruption That looks a bit weird, maybe "some corupption"? Or maybe it's just me not being a native speaker. > Other than that, it appears to cover everything. One more thing we could check is the relfilenode after we corrupted it, it should also catch the corruption then. Or is that too trivial? Michael -- Michael Banck Projektleiter / Senior Berater Tel.: +49 2166 9901-171 Fax: +49 2166 9901-100 Email: michael.banck@credativ.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
On Tue, Oct 09, 2018 at 05:14:50PM +0200, Michael Banck wrote: > Am Dienstag, den 09.10.2018, 16:54 +0200 schrieb Peter Eisentraut: >> On 06/10/2018 13:46, Michael Paquier wrote: > >>> +# Time to create a corruption > > That looks a bit weird, maybe "some corruption"? Or maybe it's just me > not being a native speaker. Okay, let's do with your suggestion. >> I would also like to see a test that runs against a cluster without >> checksums enabled. OK. I have added a test within initdb to save one initdb run. >> +# Checks cannot happen for an online cluster >> +$node->start; >> +command_fails(['pg_verify_checksums', '-D', $pgdata], >> + "checksum checks not done"); >> >> The test name should be something like "fails with online cluster". Done. I have put more thoughts into those. > One more thing we could check is the relfilenode after we corrupted it, > it should also catch the corruption then. Or is that too trivial? There is one as of v3: +# Checksum checks on single relfilenode fail +$node->command_checks_all([ 'pg_verify_checksums', '-D', $pgdata, '-r', + $relfilenode_corrupted], + 1, + [qr/Bad checksums:.*1/], + [qr/checksum verification failed/], + ''); The resulting patch is attached. Does that look good? -- Michael
Attachment
On Wed, Oct 10, 2018 at 10:50:02AM +0900, Michael Paquier wrote: > The resulting patch is attached. Does that look good? And committed. Thanks all for taking the time to review. -- Michael