More issues with pg_verify_checksums and checksum verification inbase backups
От | Michael Paquier |
---|---|
Тема | More issues with pg_verify_checksums and checksum verification inbase backups |
Дата | |
Msg-id | 20181021134206.GA14282@paquier.xyz обсуждение исходный текст |
Ответы |
Re: More issues with pg_verify_checksums and checksum verificationin base backups
Re: More issues with pg_verify_checksums and checksum verification inbase backups |
Список | pgsql-hackers |
Hi all, This is a follow-up of the following thread: https://www.postgresql.org/message-id/20181012010411.re53cwcistcpip5j@alap3.anarazel.de In a nutshell, b34e84f1 has added TAP tests for pg_verify_checksums, and the buildfarm has immediately complained about files generated by EXEC_BACKEND missing, causing pg_verify_checksums to fail for such builds. An extra issue has been noticed by Andres in the tool: it misses to check for temporary files, hence files like base/pgsql_tmp/pgsql_tmp$PID.NN.sharedfileset/1.1 can also cause the tool to fail. After a crash, those files would not be removed, and stopping the instance would still let them around. pg_verify_checksums used first a blacklist to decide if files have checksums or not. So with this approach all files should have checksums except files like pg_control, pg_filenode.map, PG_VERSION, etc. d55241a has added a first fix to silence the buildfarm for EXEC_BACKEND builds. After discussion, Andres has pointed out that some extensions may want to drop files within global/ or base/ as part of their logic. cstore_fdw was mentioned but if you look at their README that's not the case. However, I think that Andres argument is pretty good as with pluggable storage we should allow extensions to put custom files for different tablespaces. So this commit has changed the logic of pg_verify_checksums to use a whitelist, which assumes that only normal relation files have checksums: <digits> <digits>.<segment> <digits>_<forkname> <digits>_<forkname>.<segment After more discussion on the thread mentioned above, Stephen has pointed out that base backups use the same blacklist logic when verifying checksums. This has the same problem with EXEC_BACKEND-specific files, resulting in spurious warnings (that's an example!) which are confusing for the user: $ pg_basebackup -D popo WARNING: cannot verify checksum in file "./global/config_exec_params", block 0: read buffer size 5 and page size 8192 differ Folks on this thread agreed that both pg_verify_checksums and checksum verification for base backups should use the same logic. It seems to me that using a whitelist-based approach for both is easier to maintain as the patterns of files supporting checksums is more limited than files which do not support checksums. So this way we allow extensions bundling custom files to still work with pg_verify_checksums and checksum verification in base backups. Something else which has been discussed on this thread is that we should have a dedicated API to decide if a file has checksums or not, and if it should be skipped in both cases. That's work only for HEAD though, so we need to do something for HEAD and v11, which is simple. Attached is a patch I have cooked for this purpose. I have studied the file patterns opened by the backend, and we actually need to only skip temporary files and folders as those can include legit relation file names (like 1.1 for example). At the same time I have found about parse_filename_for_nontemp_relation() which is a copycat of the recently-added isRelFileName(), so we can easily shave some code by reusing it in both cases. This also takes care of the issues for temporary files, and it also fixes an extra bug coming from the original implementation which checked the file patterns without looking at the file type first, so the tool could miss some cascading paths. This should be applied to v11 and HEAD. Please feel free to comment. Thanks for reading. -- Michael
Вложения
В списке pgsql-hackers по дате отправления: