On Tue, Sep 6, 2016 at 2:18 PM, Abhijit Menon-Sen <ams@2ndquadrant.com> wrote:
> One open issue is the various assign_recovery_target_xxx functions,
> which Michael noted in his earlier review:
> [...]
> 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.
By moving all the recovery parameters to GUC, we will need to define a
hierarchy among the target types. I see no way to avoid that except by
changing the parametrization but that would be more confusing for the
users. So that will not be anymore the last one wins, but the first
one listed wins in all the ones enabled that wins. I think that we
could leverage that a little bit though and reduce the complexity of
the patch: my best advice here is to make all those recovery_target_*
parameters PGC_POSTMASTER so as they are loaded only once when the
server starts, and then we define the recovery target type used in the
startup process instead of trying to do so at GUC level. This does not
change the fact that we'd still need a hierarchy among the target
types, but it slightly reduces the complexity of the patch. And this
does not prevent further enhancements by switching them later to be
PGC_SIGHUP, really. I'd really like to see this improvement, but I
don't think that this applies to this change, which is complicated
enough, and will likely introduce its lot of bugs even after several
reviews.
> 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.
Interesting ideas, particularly the last one. Mixing both:
recovery_target_type = 'foo' # can be 'immediate'
recovery_target_value = 'value_of_foo' # does not matter for 'immediate'
> 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).
My vote goes in favor of that..
+ else if (recovery_target_lsn != 0)
+ recovery_target = RECOVERY_TARGET_LSN;
This needs to check for InvalidXLogRecPtr.
--
Michael