Hallo Michael,
> It adds an (now mandatory) --action parameter that takes either verify,
> enable or disable as argument.
I'd rather have explicit switches for verify, enable & disable, and verify
would be the default if none is provided.
> This is basically meant as a stop-gap measure in case online activation
> of checksums won't make it for v12, but maybe it is independently
> useful?
I would say yes.
> 1. Rename pg_verify_checksums to e.g. pg_checksums as it will no longer
> only verify checksums.
I'd agree to rename the tool as "pg_checksums".
> 2. Rename the scan_* functions (Michael renamed them to operate_file and
> operate_directory but I am not sure it is worth it.
Hmmm. The file is indeed scanned, and "operate" is kind of very fuzzy.
> 3. Once that patch is in, there would be a way to disable checksums so
> there'd be a case to also change the initdb default to enabled, but that
> required further discussion (and maybe another round of benchmarks).
My 0.02€ is that data safety should comes first, thus checksums should be
enabled by default.
About the patch: applies, compiles, "make check" ok.
There is no documentation.
In "scan_file", I would open RW only for enable, but keep RO for verify.
Also, the full page is rewritten... would it make sense to only overwrite
the checksum part itself?
It seems that the control file is unlinked and then rewritten. If the
rewritting fails, or the command is interrupted, the user has a problem.
Could the control file be simply opened RW? Else, I would suggest to
rename (eg add .tmp), write the new one, then unlink the old one, so that
recovering the old state in case of problem is possible.
For enable/disable, while the command is running, it should mark the
cluster as opened to prevent an unwanted database start. I do not see
where this is done.
--
Fabien.
