On 24/12/14 20:11, Alex Shulgin wrote:
> Alex Shulgin <ash@commandprompt.com> writes:
>
>>> - 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.
>
I don't buy that to be honest, most (if not all) places that would be
affected are already in diff as part of context around other renames
anyway and it just does not seem right to rename some variables and not
the others.
I still think there should be some thought given to merging the
hot_standby and standby_mode, but I think we'd need opinion of more
people (especially some committers) on this one.
>>> - 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).
Right, my bad.
>
> The FlushErrorState() call is not there because it's handled by the hook
> caller (or it can exit via ereport() with elevel==ERROR).
>
Yes I've seen that, shouldn't you FreeErrorData(edata) then though? I
guess it does not matter too much considering that you are going to
throw error and die eventually anyway once you are in that code path,
maybe just for the clarity...
-- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training &
Services