On Tue, Sep 6, 2016 at 2:18 PM, Abhijit Menon-Sen <a...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
