Re: Turning recovery.conf into GUCs

From: Alex Shulgin <ash(at)commandprompt(dot)com>
To: Petr Jelinek <petr(at)2ndquadrant(dot)com>
Cc: Andres Freund <andres(at)2ndquadrant(dot)com>, Jaime Casanova <jaime(at)2ndquadrant(dot)com>, Josh Berkus <josh(at)agliodbs(dot)com>, Peter Eisentraut <peter_e(at)gmx(dot)net>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Turning recovery.conf into GUCs
Date: 2014-12-24 19:11:08
Message-ID: 87vbl0ualv.fsf@commandprompt.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Alex Shulgin <ash(at)commandprompt(dot)com> writes:

> Petr Jelinek <petr(at)2ndquadrant(dot)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

Attachment Content-Type Size
recovery_guc_v5.6.patch.gz application/gzip 31.6 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Jim Nasby 2014-12-24 19:58:41 Re: hash_create API changes (was Re: speedup tidbitmap patch: hash BlockNumber)
Previous Message Jim Nasby 2014-12-24 18:53:06 Re: Proposal "VACUUM SCHEMA"