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
