Re: Standby accepts recovery_target_timeline setting?

On Wed, Oct 9, 2019 at 1:02 AM Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>
> Greetings,
>
> * 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
etc

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?

Regards,

--
Fujii Masao