Re: More issues with pg_verify_checksums and checksum verification in base backups

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Stephen Frost <sfrost(at)snowman(dot)net>
Cc: Andres Freund <andres(at)anarazel(dot)de>, David Steele <david(at)pgmasters(dot)net>, Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>, pgsql-hackers(at)postgresql(dot)org, andrew(at)dunslane(dot)net, daniel(at)yesql(dot)se, magnus(at)hagander(dot)net, tgl(at)sss(dot)pgh(dot)pa(dot)us
Subject: Re: More issues with pg_verify_checksums and checksum verification in base backups
Date: 2018-11-28 00:57:27
Message-ID: 20181128005727.GS1716@paquier.xyz
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Tue, Nov 27, 2018 at 06:27:57PM -0500, Stephen Frost wrote:
> * Michael Paquier (michael(at)paquier(dot)xyz) wrote:
>> Believe me or not, but we have spent so much energy on this stuff that I
>> am ready to give up on the whitelist patch and focus on other business.
>
> I would have hoped that you'd see why I was concerned from the start
> about this now that we have a released version of pg_verify_checksums
> in 11.1 that is paper-bag-worthy.

That's unrelated. Let's be clear, I still don't like the fact that we
don't use a whitelist approach, per the arguments raised upthread. The
tool also clearly lacked testing and coverage from the beginning and has
never been able to work on Windows, which was a very bad decision.
Still we need to live with that and I take my share of the blame,
willing to make this stuff work better for all.

>> - skipfile should be called after making sure that we work on a file.
>
> It's not clear to me what this is referring to, exactly...? Can you
> clarify?

Please see 0002 attached, which moves the call to skipfile() where I
think it should go.

>> - it is necessary to exclude EXEC_BACKEND files.
>
> Agreed, they should be added to the exclude list.

Base backups are impacted as well as this causes spurious warnings.

Attached are two patches to fix all the mess:
- 0001 is a revert of the whitelist, minus the set of regression tests
checking after corrupted files and empty files.
- 0002 is a fix for all the issues reported on this thread, with tests
added (including the tablespace test from Michael Banck):
-- Base backups gain EXEC_BACKEND files in their warning filters.
-- pg_verify_checksums gains the same files.
-- temporary files are filtered out.
-- pg_verify_checksums performs filtering checks only on regular files,
not on paths.

0001 and 0002 need to be merged as 0001 would cause the buildfarm to
turn red on Windows if applied alone. Can you know see my point?
--
Michael

Attachment Content-Type Size
0001-Switch-pg_verify_checksums-back-to-a-blacklist.patch text/x-diff 4.4 KB
0002-Fix-set-of-issues-with-pg_verify_checksums-and-base-.patch text/x-diff 8.4 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2018-11-28 01:00:00 Re: Handling of REGRESS_OPTS in MSVC for regression tests
Previous Message Stephen Frost 2018-11-28 00:51:03 Re: Minor typo