Greetings, * Michael Paquier (mich...@paquier.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
signature.asc
Description: PGP signature
