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

On Tue, Oct 8, 2019 at 10:58 PM Stephen Frost <sfrost@snowman.net> wrote:
>
> 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.

Yeah, I agree.

> 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.

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.

Regards,

-- 
Fujii Masao