Re: Proposal for changes to recovery.conf API

On Mon, Jan 2, 2017 at 6:14 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On 20 December 2016 at 15:11, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> On 20 December 2016 at 15:03, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>
>>> API for crash recovery will never be changed. That is, crash recovery needs
>>> neither recovery.trigger nor standby.trigger. When the server starts a crash
>>> recovery without any trigger file, any recovery parameter settings in
>>> postgresql.conf are ignored. Right?
>>
>> Yes. There are no conceptual changes, just the API.
>>
>> The goals are: visibility and reloading of recovery parameters,
>> removal of special case code.
>
> OK, so here's the patch, plus doc cleanup patch.

Thanks for the patch!

> 5. recovery.conf parameters are all moved to postgresql.conf, with these changes

In current design of the patch, when recovery parameters are misconfigured
(e.g., set recovery_target_timeline to invalid timeline id) and
the configuration file is reloaded, the startup process emits FATAL error and
the server goes down. I don't think this is fine. Basically even
misconfiguration of the parameters should not cause the server crash.
If invalid settings are supplied, I think that we just should warn them
and ignore those new settings, like current other GUC is. Thought?

- if (PrimaryConnInfo == NULL && recoveryRestoreCommand == NULL)
+ if (SenderConnInfo == NULL && recoveryRestoreCommand == NULL)

Seems "SenderConnInfo == NULL" should be changed to "SenderConnInfo[0] == '\0'".
recoveryRestoreCommand, as well.

Regards,

--
Fujii Masao