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
