Re: Standby accepts recovery_target_timeline setting?

Greetings,

* Fujii Masao (masao(dot)fujii(at)gmail(dot)com) wrote:
> On Wed, Oct 9, 2019 at 1:02 AM Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > * Fujii Masao (masao(dot)fujii(at)gmail(dot)com) wrote:
> > > On Tue, Oct 8, 2019 at 10:58 PM Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > > > Having these options end up set but then hacking all of the other code
> > > > that looks at them to check if we're actually in recovery or not would
> > > > end up being both confusing to users as well as an ongoing source of
> > > > bugs (which has already been made clear by the fact that we're having
> > > > this discussion...). Wouldn't that also mean we would need to hack the
> > > > 'show' code, to blank out the recovery_target_name variable if we aren't
> > > > in recovery? Ugh.
> > >
> > > Isn't this overkill? This doesn't seem the problem only for recovery-related
> > > settings. We have already have the similar issue with other settings.
> > > For example, log_directory parameter is ignored when logging_collector is
> > > not enabled. But SHOW log_directory reports the setting value even when
> > > logging_collector is disabled. This seems the similar issue and might be
> > > confusing, but we could live with that.
> >
> > I agree it's a similar issue. I disagree that it's actually sensible
> > for us to do so and would rather contend that it's confusing and not
> > good.
> >
> > We certainly do a lot of smart things in PG, but showing the value of
> > variables that aren't accurate, and we *know* they aren't, hardly seems
> > like something we should be saying "this is good and ok, so let's do
> > more of this."
> >
> > I'd rather argue that this just shows that we need to come up with a
> > solution in this area. I don't think it's *as* big of a deal when it
> > comes to logging_collector/log_directory because, at least there, you
> > don't even start to get into the same code paths where it matters, like
> > you end up doing with the recovery targets and crash recovery, so the
> > chances of bugs creeping in are less in the log_directory case.
> >
> > I still don't think it's great though and, yes, would prefer that we
> > avoid having log_directory set when logging_collector is in use.
>
> There are other parameters having the similar issue, for example,
> - parameters for SSL connection when ssl is disabled
> - parameters for autovacuum activity when autovacuum is disabled
> - parameters for Hot Standby when hot_standby is disabled

I agree that those would also be nice to improve with some indication
that those features are disabled, but, again, as I said above, while
they're confusing they at least don't *also* lead to bugs where the code
itself is confused about the state of the system, because we don't have
two different major ways of getting to the same code.

> Yeah, it's better to make SHOW command handle these parameters
> "less confusing". But I cannot wait for the solution for them before
> fixing the original issue in v12 (i.e., the issue where restore_command
> can be executed even in crash recovery). So, barring any objection,
> I'd like to commit the patch that I attached upthread, soon.
> The patch prevents restore_command and recovery_end_command
> from being executed in crash recovery. Thought?

I'm not suggesting that we fix everything in this area in a patch that
we back-patch, and I haven't intended to imply that at all throughout
this, so this argument doesn't really hold. I do think we should fix
this issue, where we've seen bugs from the confusion, in the right way
by realizing that this is a direction that's prone to cause bugs.

Thanks,

Stephen