Re: Continue work on changes to recovery.conf API

Hello

thank you for review!

>>  - if present both standby.signal and recovery.signal - we use standby mode
>>  (this design from previous thread)
>
> Didn't find the discussion on that and don't understand the reasons, but
> seems OK and easy to change if we don't like it.
I meant this was implemented in previous proposed patch and i do not changed this. I didn't find the discussion on that too...

>>  - recovery_target (immediate), recovery_target_name, recovery_target_time, recovery_target_xid, recovery_target_lsn are replaced to recovery_target_type and recovery_target_value (was discussed and changed in previous thread)
>
> I think this was the major point of contention. I reread the old
> thread, and it's still not clear why we need to change this. _type and
> _value look like an EAV system to me. GUC variables should be
> verifiable independent of another variable. The other idea of using
> only one variable seems better, but using two variables seems like a
> poor compromise between one and five.

> No, they MUST be independently verifiable. The interactions between the check_xxx functions in this patch are utterly unsafe.

Understood, thank you.
I will implement set of current five recovery_target* settings and would like to leave reorganization for futher pathes.

Not sure about one thing. All recovery_target settings change one internal recoveryTarget variable. GUC infrastructure will guarantee behavior if user erroneously set multiple different recovery_target*? I mean check_* and assign_* will be called in order and so latest config line wins?

>> - trigger_file was renamed to promote_signal_file for clarify (my change, in prev thread was removed)
>
> OK to add the "promote" prefix, but I'd prefer to keep the "trigger"
> name. There is a lot of discussion and knowledge around that. Seems
> unnecessary to change.
Ok, will change to promote_trigger_file

>>  - pg_basebackup with -R (--write-recovery-conf) option will create standby.signal file and append primary_conninfo and primary_slot_name (if was used) to postgresql.auto.conf instead of writing new recovery.conf
>
> I wonder if that would cause any problems. The settings in
> postgresql.auto.conf are normally not PGC_POSTMASTER, otherwise you
> couldn't use ALTER SYSTEM to put them there. Maybe it's OK.
Actually we can use ALTER SYSTEM to put PGC_POSTMASTER settings. I tried now "alter system set max_connections to 300;", postgresql.auto.conf was changed and affect max_connections during database start.
Of course, we can not reload PGC_POSTMASTER settings, but during start we call regular ParseConfigFile and can override PGC_POSTMASTER.

regards, Sergei