Bonjour Michaël,
>>> 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.
>
> Okay, noted for the separate switches. But I don't agree with the
> point of assuming that --verify should be enforced if no switches are
> defined. That feels like a trap for newcomers of this tool..
Hmmm. It does something safe and useful, especially if it also works
online (patch pending), and the initial tool only does checking. However,
I'd be okay for no default.
>> 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.
>
> You have pretty much the same class of problems if you attempt to
> start a cluster on which pg_rewind or the existing pg_verify_checksums
> is run after these have scanned the control file to make sure that
> they work on a cleanly-stopped instance. In short, this is a deal
> between code simplicity and trying to have the tool outsmart users in
> the way users use the tool. I tend to prefer keeping the code simple
> and not worry about cases where the users mess up with their
> application, as there are many more things which could go wrong.
Hmmm. I do not buy the comparison.
A verify that fails is not a big problem, you can run it again. If
pg_rewind fails, you can probably run it again as well, the source is
probably still consistent even if it has changed, and too bad for the
target side, but it was scheduled to be overwritten anyway.
However, a tool which overwrites files beyond the back of a running server
is a recipee for data-loss, so I think it should take much more care, i.e.
set the server state into some specific safe state.
About code simplicity: probably there is, or there should be, a
change-the-state function somewhere, because quite a few tools could use
it?
--
Fabien.
