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

Greetings,

* Michael Paquier (michael(at)paquier(dot)xyz) wrote:
> 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.

I don't agree that it's unrelated and I'm disappointed that you feel it
is, but I'm not going to belabor the point.

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

Alright, on a quick glance that seems ok.

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

Right- could you please also add comments around the two lists to make
other hackers aware that the list shows up in two places and that any
changes should be made in both places..?

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

Yes, I think they could be merged to address that, though I'm not sure
that it's necessairly a huge deal either, if they're going to be pushed
together.

Thanks!

Stephen