Re: Proposal for changes to recovery.conf API

On 06/09/16 07:18, Abhijit Menon-Sen wrote:
>
> One open issue is the various assign_recovery_target_xxx functions,
> which Michael noted in his earlier review:
>
> +static void
> +assign_recovery_target_xid(const char *newval, void *extra)
> +{
> +	recovery_target_xid = *((TransactionId *) extra);
> +
> +	if (recovery_target_xid != InvalidTransactionId)
> +		recovery_target = RECOVERY_TARGET_XID;
> +	else if (recovery_target_name && *recovery_target_name)
> +		recovery_target = RECOVERY_TARGET_NAME;
> +	else if (recovery_target_time != 0)
> +		recovery_target = RECOVERY_TARGET_TIME;
> +	else if (recovery_target_lsn != 0)
> +		recovery_target = RECOVERY_TARGET_LSN;
> +	else
> +		recovery_target = RECOVERY_TARGET_UNSET;
> +}
>
> (Note how recovery_target_lsn caused this—and three other functions
> besides—to grow an extra branch.)
>
> I don't like this code, but I'm not yet sure what to replace it with. I
> think we should address the underlying problem—that the UI doesn't map
> cleanly to what the code wants. There's been some discussion about this
> earlier, but not any consensus that I could see.
>
> Do we want something like this (easy to implement and document, perhaps
> not especially convenient to use):
>
>     recovery_target = 'xid'     # or 'time'/'name'/'lsn'/'immediate'
>     recovery_target_xid = xxx?  # the only setting we care about now
>     recovery_target_otherthings = parsed_but_ignored
>
> Or something like this (a bit harder to implement):
>
>     recovery_target = 'xid:xxx' # or 'time:xxx' etc.
>

Personally, I never liked the fact that we have several config variables 
for this and then the last one is chosen (even when it was in 
recovery.conf). We support one recovery_target at a time so it would 
make sense to have single config option for it IMHO.

So +1 on the recovery_target = 'xid:xxx' idea.

-- 
   Petr Jelinek                  http://www.2ndQuadrant.com/
   PostgreSQL Development, 24x7 Support, Training & Services