Alex Shulgin <ash@commandprompt.com> writes:
> Petr Jelinek <petr@2ndquadrant.com> writes:
>>
>> I had a quick look, the patch does not apply cleanly anymore but it's
>> just release notes so nothing too bad.
>
> Yes, there were some ongoing changes that touched some parts of this and
> I must have missed the latest one (or maybe I was waiting for it to
> settle down).
The rebased version is attached.
>> - the StandbyModeRequested and StandbyMode should be lowercased like
>> the rest of the GUCs
>
> Yes, except that standby_mode is linked to StandbyModeRequested, not the
> other one. I can try see if there's a sane way out of this.
As I see it, renaming these will only add noise to this patch, and there
is also ArchiveRecoveryRequested / InArchiveRecovery. This is going to
be tricky and I'm not sure it's really worth the effort.
>> - The whole CopyErrorData and memory context logic is not needed in
>> check_recovery_target_time() as the FlushErrorState() is not called
>> there
>
> You must be right. I recall having some trouble with strings being
> free'd prematurely, but if it's ERROR, then there should be no need for
> that. I'll check again.
Hrm, after going through this again I'm pretty sure that was correct:
the only way to obtain the current error message is to use
CopyErrorData(), but that requires you to switch back to non-error
memory context (via Assert).
The FlushErrorState() call is not there because it's handled by the hook
caller (or it can exit via ereport() with elevel==ERROR).
>> - 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.
>
> We can move it at top of CheckStartingAsStandby() obviously.
Moved.
--
Alex
