Re: pgsql: Add TAP tests for pg_verify_checksums

Greetings,

* Michael Paquier (michael(at)paquier(dot)xyz) wrote:
> On Wed, Oct 17, 2018 at 05:30:05PM -0400, Andrew Dunstan wrote:
> > Fine by me.
>
> Thanks. This is now committed after some tweaks to the comments, a bit
> earlier than I thought first.

I just saw this committed and I'm trying to figure out why we are
creating yet-another-list when it comes to deciding what should be
checksum'd and what shouldn't be.

Specifically, pg_basebackup (or, really,
src/backend/replication/basebackup.c) has 'is_checksummed_file' which
operates differently from pg_verify_checksum with this change, and that
seems rather wrong.

Maybe we need to fix both places but I *really* don't like this approach
of "well, we'll just guess if the file should have a checksum or not"
and it certainly seems likely that we'll end up forgetting to update
this when we introduce things in the future which have checksums (which
seems pretty darn likely to happen).

I also categorically disagree with the notion that it's ok for
extensions to dump files into our tablespace diretories or that we
should try to work around random code dumping extra files in the
directories which we maintain- it's not like this additional code will
actually protect us from that, after all, and it's foolish to think we
really could protect against that.

Basically, I think this commit should be reverted, we should go back to
having a blacklist, update it in both places (and add comments to both
places to make it clear that this list exists in two different places)
and add code to handle the temp tablespace case explicitly.

Even better would be to find a way to expose the list and the code for
walking the directories and identifying the files which contain
checksums, so that we're only doing that in one place instead of
multiple different places.

Thanks!

Stephen