On Thu, Dec 27, 2018 at 2:15 AM Michael Paquier <mich...@paquier.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/>
