On 12/12/14 16:34, Alex Shulgin wrote:
> Alex Shulgin <ash@commandprompt.com> writes:
>
>> Alex Shulgin <ash@commandprompt.com> writes:
>>>
>>> Here's an attempt to revive this patch.
>>
>> Here's the patch rebased against current HEAD, that is including the
>> recently committed action_at_recovery_target option.
>>
>> The default for the new GUC is 'pause', as in HEAD, and
>> pause_at_recovery_target is removed completely in favor of it.
>>
>> I've also taken the liberty to remove that part that errors out when
>> finding $PGDATA/recovery.conf. Now get your rotten tomatoes ready. ;-)
>
> This was rather short-sighted, so I've restored that part.
>
> Also, rebased on current HEAD, following the rename of
> action_at_recovery_target to recovery_target_action.
>
Hi,
I had a quick look, the patch does not apply cleanly anymore but it's
just release notes so nothing too bad.
I did not do any testing yet, but I have few comments about the code:
- the patch mixes functionality change with the lowercasing of the
config variables, I wonder if those could be separated into 2 separate
diffs - it would make review somewhat easier, but I can live with it as
it is if it's too much work do separate into 2 patches
- the StandbyModeRequested and StandbyMode should be lowercased like the
rest of the GUCs
- I am wondering if StandbyMode and hot_standby should be merged into
one GUC if we are breaking backwards compatibility anyway
- Could you explain the reasoning behind:
+ if ((*newval)[0] == 0)
+ xid = InvalidTransactionId;
+ else
in check_recovery_target_xid() (and same check in
check_recovery_target_timeline()), isn't the strtoul which is called
later enough?
- The whole CopyErrorData and memory context logic is not needed in
check_recovery_target_time() as the FlushErrorState() is not called there
- The new code in StartupXLOG() like
+ if (recovery_target_xid_string != NULL)
+ InitRecoveryTarget(RECOVERY_TARGET_XID);
+
+ if (recovery_target_time_string != NULL)
+ InitRecoveryTarget(RECOVERY_TARGET_TIME);
+
+ if (recovery_target_name != NULL)
+ InitRecoveryTarget(RECOVERY_TARGET_NAME);
+
+ if (recovery_target_string != NULL)
+ InitRecoveryTarget(RECOVERY_TARGET_IMMEDIATE);
would probably be better in separate function that you then call
StartupXLOG() as StartupXLOG() is crazy long already so I think it's
preferable to not make it worse.
- I wonder if we should rename trigger_file to standby_tigger_file
-- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training &
Services
