Re: Proposal for changes to recovery.conf API

On Thu, Sep 1, 2016 at 4:01 AM, Abhijit Menon-Sen <ams@2ndquadrant.com> wrote:
> At 2016-08-31 17:15:59 +0100, simon@2ndquadrant.com wrote:
>>
>> * Recovery parameters would now be part of the main postgresql.conf
>> infrastructure
>> Any parameters set in $DATADIR/recovery.conf will be read after the
>> main parameter file, similar to the way that postgresql.conf.auto is
>> read.
>> (Abhijit)
>>
>> * Parameters
>> All of the parameters formerly set in recovery.conf can be set in
>> postgresql.conf using RELOAD
>> These parameters will have no defaults in postgresql.conf.sample
>> Setting them has no effect during normal running, or once recovery ends.
>>  https://www.postgresql.org/docs/devel/static/archive-recovery-settings.html
>>  https://www.postgresql.org/docs/devel/static/recovery-target-settings.html
>>  https://www.postgresql.org/docs/devel/static/standby-settings.html
>> (Abhijit)
>
> I've attached a WIP patch for the above (recovery_guc_v20160831.patch).
> This was based on the unite_recoveryconf_postgresqlconf_v3.patch posted
> by Fujii Masao.
>
> Unfortunately, some parts conflict with the patch that Simon just posted
> (e.g., his patch removes trigger_file altogether, whereas mine converts
> it into a GUC along the lines of the original patch). Rather than trying
> to untangle that right now, I'm posting what I have as-is, and I'll post
> an updated version tomorrow.

-       else if (recoveryTarget == RECOVERY_TARGET_XID)
-           ereport(LOG,
-                   (errmsg("starting point-in-time recovery to XID %u",
-                           recoveryTargetXid)));
User loses information if those logs are removed.

+       {"recovery_min_apply_delay", PGC_SIGHUP, WAL_RECOVERY_TARGET,
+           gettext_noop("Sets the minimum delay to apply changes
during recovery."),
+           NULL,
+           GUC_UNIT_MS
+       },
What's the point of having them all as SIGHUP? The startup process
does not reload GUC parameters with ProcessConfigFile(), it works on
recoveryWakeupLatch though. I'd rather let them as PGC_POSTMASTER to
begin with as that's the safest approach, and then get a second patch
that processes ProcessConfigFile in the startup process and switch
some of them to PGC_SIGHUP. Recovery targets should not be SIGHUP, but
recovery_min_apply_delay applies definitely applies to that, as well
as archive_cleanup_command or restore_command.

+static void
+assign_recovery_target_name(const char *newval, void *extra)
+{
+   if (recovery_target_xid != InvalidTransactionId)
+       recovery_target = RECOVERY_TARGET_XID;
+   else if (newval[0])
+       recovery_target = RECOVERY_TARGET_NAME;
+   else if (recovery_target_time != 0)
+       recovery_target = RECOVERY_TARGET_TIME;
+   else
+       recovery_target = RECOVERY_TARGET_UNSET;
+}
That's brittle to put that in the GUC machinery... The recovery target
type should be determined only once, if archive recovery is wanted at
the beginning of the startup process once and for all, and just be set
up within the startup process. If multiple recovery_target_*
parameters are set, we should just define the target type in order of
priority, instead of the-last-one-wins that is currently present.
-- 
Michael