Re: Turning recovery.conf into GUCs

On 12/12/14 16:34, Alex Shulgin wrote:
> Alex Shulgin <ash(at)commandprompt(dot)com> writes:
>
>> Alex Shulgin <ash(at)commandprompt(dot)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