Re: Proposal for changes to recovery.conf API

Hi.

Here's an updated version of my patch, which now applies on top of the
patch that Simon posted earlier (recovery_startup_r10_api.v1b.patch).

A few notes:

1. I merged in recovery_target_lsn as a new GUC setting.
2. I fixed various minor nits in the earlier patch, not worth mentioning
individually.
3. I haven't added back trigger_file, because Simon's patch removes it.
I can add it back in separately after discussion (otherwise Simon's
and my patches will conflict).
4. I've tested this to the extent that setting things in postgresql.conf
works, and recovery.conf is still read if it exists, and so on.

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.

Alternatively, the do-nothing option is to move the tests from guc.c to
StartupXLOG and do it only once in some defined order (which would also
break the current last-mention-wins behaviour).

Thoughts? (I've added Fujii to the Cc: list, in case he has any
comments, since this is based on his earlier patch.)

-- Abhijit