Re: Standby accepts recovery_target_timeline setting? - Mailing list pgsql-hackers

Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Mon, Oct 7, 2019 at 9:14 AM Fujii Masao <masao.fujii@gmail.com> wrote:
> > Agreed, too. Do you have any idea to implement that? I've not found out
> > "smart" way to do that yet.
> >
> > One idea is, as Michael suggested, to use SetConfigOption() for all the
> > archive recovery parameters at the beginning of the startup process as follows,
> > to forcibly set the default values if crash recovery is running. But this
> > seems not smart for me.
> >
> > SetConfigOption("restore_command", ...);
> > SetConfigOption("archive_cleanup_command", ...);
> > SetConfigOption("recovery_end_command", ...);
> > ...
> >
> > Maybe we should make GUC mechanism notice signal files and ignore
> > archive recovery-related parameters when none of those files exist?
> > This change seems overkill at least in v12, though.
>
> I think this approach is going in the wrong direction. In every other
> part of the system, it's the job of the code around the GUC system to
> use parameters when they're relevant and ignore them when they should
> be ignored. Deciding that the parameters that were formerly part of
> recovery.conf are an exception to that rule and that the GUC system is
> responsible for making sure they're set only when we pay attention to
> them seems like it's bringing back or exacerbating a code-level split
> between recovery.conf parameters and postgresql.conf parameters when,
> meanwhile, we've been wanting to eradicate that split so that the
> things we allow for postgresql.conf parameters -- e.g. changing them
> while they are running -- can be applied to these parameters also.

I don't think we necessairly need to be thinking about trying to
eliminate all differences between certain former recovery.conf settings
and things like work_mem, even as we make it such that those former
settings can be changed while we're running.

> I don't particularly like the use of SetConfigOption() either,
> although it does have some precedent in autovacuum, for example.

It's pretty explicitly the job of SetConfigOption to manage the fact
that only certain options can be set at certain times, as noted at the
top of guc.h where we're talking about GUC contexts (and which
SetConfigOption references as being what it's job is to manage-
guc.c:6776 currently).

> Generally, it's right and proper that the GUC system sets the
> variables to which the parameters it controls are tied -- and then the
> rest of the code has to do the right thing around that. It sounds like
> the patch that got rid of recovery.conf wasn't considered carefully
> enough, and missed the fact that it was introducing some inadvertent
> behavior changes. That's too bad, but let's not overreact. It seems
> totally fine to me to just add ad-hoc checks that rule out
> inappropriately relying on these parameters while performing crash
> recovery - and be done with it.

The patch that got rid of recovery.conf also removed the inherent
understanding and knowledge that there are certain options that can only
be set (and make sense ...) at certain times- namely, when we're doing
recovery.  Having these options set at other times is entirely wrong and
will be confusing to both users, and, as seen, code.  From a user
perspective, what happens when you've started up PG as a primary, since
you don't have a signal file in place to indicate that you're doing
recovery, and you have a recovery_target set, so some user does
"show recovery_target_name" and sees a value?  How is that sensible?

Those options should only be set when we're actually doing recovery,
which is governed by the signal file.  Recovery is absolutely a specific
kind of state that the system is in, not unlike postmaster, we've even
got a specific pg_is_in_recovery() function for it.

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.

Thanks,

Stephen