> I changed that to the switches -c/--verify (-c for check as -v is taken,
> should it be --check as well? I personally like verify better),
> -d/--disable and -e/--enable.
I agree that checking the checksum sounds repetitive, but I think that for
consistency --check should be provided.
About the patch: applies, compiles, global & local "make check" are ok.
There is still no documentation.
I think that there is a consensus about renaming the command.
The --help string documents --action which does not exists anymore.
The code in "updateControlFile" seems to allow to create the file
(O_CREAT). I do not think that it should, it should only apply to an
existing file.
ISTM that some generalized version of this function should be in
"src/common/controldata_utils.c" instead of duplicating it from command to
command (as suggested by Michaël as well).
In "scan_file" verbose output, ISTM that the checksum is more computed
than enabled on the file. It is really enabled at the cluster level in the
end.
Maybe there could be only one open call with a ?: for RO vs RW.
Non root check: as files are only manipulated RW, ISTM that there is no
reason why the ownership would be changed, so I do not think that this
constraint is useful.
There is kind of a copy paste for enabling/disabling, I'd consider
skipping the scan when not necessary and merge both branches.
>> Also, the full page is rewritten... would it make sense to only overwrite
>> the checksum part itself?
>
> So just writing the page header? I find that a bit scary and don't
> expect much speedup as the OS would write the whole block anyway I
> guess? I haven't touched that yet.
Possibly the OS would write its block size, which is not necessary the
same as postgres page size?
>> 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.
>
> I have mostly taken the pg_rewind code here; if there was a function
> that allowed for safe offline changes of the control file, I'd be happy
> to use it but I don't think it should be this patch to invent that.
It is reinventing it somehow by duplicating the stuff anyway. I'd suggest
a separate preparatory patch to do the cleanup.
--
Fabien.
