Re: unite recovery.conf and postgresql.conf

On Fri, Sep 9, 2011 at 13:15, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Fri, Sep 9, 2011 at 7:21 PM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>> On Fri, Sep 9, 2011 at 11:56, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>> Hi,
>>>
>>> http://archives.postgresql.org/pgsql-hackers/2010-12/msg02343.php
>>>
>>> In previous discussion, we've reached the consensus that we should unite
>>> recovery.conf and postgresql.conf. The attached patch does that. The
>>> patch is WIP, I'll have to update the document, but if you notice something,
>>> please feel free to comment.
>>>
>>> This patch allows us to specify recovery parameters like primary_conninfo
>>> in postgresql.conf. You can change some of them without restarting
>>> the server (i.e., by using "pg_ctl reload") and see the current settings by
>>> SHOW command. But note that recovery.conf is still required as a status
>>> file for archive recovery and standby server even if you put all the settings
>>> in postgresql.conf. Recovery parameters in postgresql.conf only have effect
>>> when recovery.conf exists.
>>
>> Why exactly do we need to keep recovery.conf in this case?
>
> PostgreSQL automatically reinitializes after a backend crash. Imagine the case
> where this happens after archive recovery. Since restore_command is left set
> in postgresql.conf and it still has effect (if we've completely removed
> recovery.conf), the reinitialization will makes the server go into
> archive recovery
> mode again unexpectedly.
>
> We can avoid this problem by using recovery.conf. At the end of
> archive recovery,
> recovery.conf is renamed to recovery.done. So when the reinitialization happens,
> recovery.conf doesn't exist and restore_command has no effect. Which prevents
> the server from going into archive recovery mode again.

Ah, ugh. Good point.

I have to wonder though, if it wouldn't be less confusing to just get
rid of recovery.conf and use a *different* file for this. Just to make
it clear it's not a config file, but just a boolean exists/notexists
state.

>>> For backward compatibility, this patch still allows us to specify recovery
>>> parameters in recovery.conf. But, as in past years, you cannot reload
>>> recovery.conf and see the current settings by SHOW command. We should
>>> recommend to put all the settings in postgresql.conf and empty recovery.conf,
>>> but you can also put all recovery parameters in recovery.conf.
>>
>> Perhaps we should just have people use recovery.conf as an include
>> file in this case?
>
> Yeah, that's my first idea, but I've given up adopting that. The problem is that
> recovery.conf is renamed to recovery.done at the end of recovery. This means
> that all the settings in recovery.conf are reset when archive recovery ends.
> If you run "pg_ctl reload" after archive recovery, you'll get something like the
> following error message and the reload will always fail;
>
>    LOG:  parameter "standby_mode" cannot be changed without
> restarting the server

Good point. And I believe another argument for actually using
completely different files ;)

>> As a whole, I'm not sure I like the idea of having such critical
>> parameters in two different places. Do we really need to care about
>> the backwards compatibility of something like that this much?
>
> I don't have strong opinion about this. But in previous discussion, Simon argued
> that we should.
> http://archives.postgresql.org/pgsql-hackers/2010-10/msg00017.php

I see his argument, but I think reduced confusion is a more important one.

>>> If the same parameter is specified in both file, the setting in recovery.conf
>>> overrides that in postgresql.conf. In this case, SHOW command displays
>>> the settings in postgresql.conf even though they are not used at all. Even if
>>
>> Now, *that* is just broken, IMHO. The SHOW command should show what is
>> actually in use, period. Everything is guaranteed to confuse the hell
>> out of anybody trying to use it.
>
> I'm afraid that we have to add very complicated code to fix that problem.
> Though of course we can easily fix the problem if we don't care about
> the backward compatibility.

That is an even bigger reason to drop backwards compatibility. Unless
someone else can come up with a neat way of fixing it.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/