Re: Continue work on changes to recovery.conf API

On 29/08/2018 17:43, Sergei Kornilov wrote:
> Current patch moves recovery.conf settings into GUC system:
> - if startup process found recovery.conf - it throw error

OK

> - recovery mode is turned on if new special file recovery.signal found

OK

> - standby_mode setting was removed. Standby mode can be enabled if startup found new special file standby.signal

OK

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

> - recovery parameters recovery_target_inclusive, recovery_target_timeline, recovery_target_action are new GUC without logic changes

OK

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

I propose to move this patch forward, keep the settings as they are. It
can be another patch to rename or reshuffle them.

> - restore_command, archive_cleanup_command, recovery_end_command, primary_conninfo, primary_slot_name and recovery_min_apply_delay are just new GUC

OK

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

> - all new GUC are PGC_POSTMASTER (discussed in prev thread)

OK

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

> - appropriate changes in tests and docs

looks good

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services