Re: Offline enabling/disabling of data checksums

On Thu, Dec 27, 2018 at 2:15 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:

> On Wed, Dec 26, 2018 at 07:43:17PM +0100, Fabien COELHO wrote:
> >> 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..
>

Defaulting to the choice that makes no actual changes to the data surely is
the safe choice,a nd not a trap :)

That said, this would probably be our first tool where you switch it
between readonly and rewrite mode with just a switch, woudn't it? All other
tools are either read-only or read/write at the *tool* level, not the
switch level.

That in itself would be an argument for making it a separate tool. But not
a very strong one I think, I prefer the single-tool-renamed approach as
well.

There's plenty enough precedent for the "separate switches and a default
behaviour if none is specified" in other tools though, and I don't think
that's generally considered a trap.

So count me in the camp for separate switches and default to verify. If one
didn't mean that, it's only a quick Ctrl-C away with no damage done.

> 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.
>

I think it comes down to what the outcome is. If we're going to end up with
a corrupt database (e.g. one where checksums aren't set everywhere but they
are marked as such in pg_control) then it's not acceptable. If the only
outcome is the tool gives an error that's not an error and if re-run it's
fine, then it's a different story.

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/>
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>