On 2015-03-15 14:51:46 +0100, Magnus Hagander wrote: > On Sun, Mar 15, 2015 at 2:27 PM, Andres Freund <andres@2ndquadrant.com> > wrote: > > > On 2015-03-12 15:52:02 +0100, Andres Freund wrote: > > > /* > > > * Override any inconsistent requests. Not that this is a change > > > * of behaviour in 9.5; prior to this we simply ignored a request > > > * to pause if hot_standby = off, which was surprising behaviour. > > > */ > > > if (recoveryTargetAction == RECOVERY_TARGET_ACTION_PAUSE && > > > recoveryTargetActionSet && > > > standbyState == STANDBY_DISABLED) > > > recoveryTargetAction = RECOVERY_TARGET_ACTION_SHUTDOWN; > > > > While it's easy enough to fix I rather dislike the whole intent here > > though. *Silently* switching the mode of operation in a rather > > significant way seems like a bad idea to me. At the very least we need > > to emit a LOG message about this; but I think it'd be much better to > > error out instead. > > > > <9.5's behaviour was already quite surprising. But changing things to a > > different surprising behaviour seems like a bad idea. > > > > +1. Especially for "sensitive" operations like this, having > predictable-behavior-or-error is usually the best choice.
Yea.
Looking further, it's even worse right now. We'll change the target to shutdown when hot_standby = off, but iff it was set in the config file. But the default value is (and was, although configured differently) documented to be 'pause'; so if it's not configured explicitly we still will promote. At least I can't read that out of the docs.
Personally I think we just should change the default to 'shutdown' for all cases. That makes documentation and behaviour less surprising. And makes experimenting less dangerous, since you can just start again.
+1. These things need to be clear. Given the consequences of getting it wrong, surprising behavior can be quite dangerous.