Re: Turning recovery.conf into GUCs - Mailing list pgsql-hackers

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