unite recovery.conf and postgresql.conf

On Fri, Sep 9, 2011 at 11:56, Fujii Masao <masao.fujii@gmail.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?

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?

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?

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.

you change the settings in postgresql.conf and reload it, they have no effect
because the settings in recovery.conf are used preferentially.

These limitations on recovery.conf might confuse users. But since most
users will put all the settings in postgresql.conf if we recommend to do that,
I don't think that it's worth complicating the source code for getting rid of
those limitations.

I think we should just get rid of supporting them in recovery.conf
completely. Recovery settings are critical, and making that confusing
to the user is not a good thing.

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

On Fri, Sep 9, 2011 at 7:21 PM, Magnus Hagander <magnus@hagander.net> wrote:

On Fri, Sep 9, 2011 at 11:56, Fujii Masao <masao.fujii@gmail.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.

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

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

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.

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Fri, Sep 9, 2011 at 13:15, Fujii Masao <masao.fujii@gmail.com> wrote:

On Fri, Sep 9, 2011 at 7:21 PM, Magnus Hagander <magnus@hagander.net> wrote:

On Fri, Sep 9, 2011 at 11:56, Fujii Masao <masao.fujii@gmail.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/

Magnus Hagander <magnus@hagander.net> writes:

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.

+1. If it's not a configuration file anymore, it shouldn't be called
one.

regards, tom lane

On Fri, Sep 9, 2011 at 3:05 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Magnus Hagander <magnus@hagander.net> writes:

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.

+1.  If it's not a configuration file anymore, it shouldn't be called
one.

+1 to rename file

+1 to overall concept, just thinking same myself, not looked at patch yet

--
 Simon Riggs                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

On 9/9/11 7:05 AM, Tom Lane wrote:

Magnus Hagander <magnus@hagander.net> writes:

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.

+1. If it's not a configuration file anymore, it shouldn't be called
one.

I'm in favor of this. People are sufficiently confused by the existing
behavior that we're not going to confuse them further by changing it.

In fact: currently we have a "trigger file" concept in recovery.conf.
Might we unite the trigger file concept with the concept of having a
file to indicate that server is a replica on startup?

i.e. in postgresql.conf we have two settings:

replication_status = {master,standby}
failover_trigger_file = '' #defaults to $PGDATA/master_trigger

If replication_status = master, then the server is a standalone or
master node.

If replication_status = standby, then the server is a PITR, warm
standby, or hot standby replica.

If the failover_trigger_file is created and detected, then PostgreSQL
will reload in master mode. replication_status will get set to "master"
so that it's easy to tell this from the command line.

The above would be a lot easier to comprehend than the current behavior,
and would allow us all of the same options the current behavoir does.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Fri, Sep 9, 2011 at 6:40 PM, Josh Berkus <josh@agliodbs.com> wrote:

I'm in favor of this.  People are sufficiently confused by the existing
behavior that we're not going to confuse them further by changing it.

Fwiw as someone who *was* confused previously, it now makes perfect
sense to me. "We have postgres.conf which always applies and then
recovery.conf which can have all the same options but they only apply
during recover". That's much clearer than "we have two configuration
files with two disjoint sets of options and good luck remembering
which options belong in which file". And it still serves a useful
purpose if you have options like recovery_target that you only want to
apply during recovery and then plan to remove.

--
greg

On Sat, Sep 10, 2011 at 01:07, Greg Stark <stark@mit.edu> wrote:

On Fri, Sep 9, 2011 at 6:40 PM, Josh Berkus <josh@agliodbs.com> wrote:

I'm in favor of this.  People are sufficiently confused by the existing
behavior that we're not going to confuse them further by changing it.

Fwiw as someone who *was* confused previously, it now makes perfect
sense to me. "We have postgres.conf which always applies and then
recovery.conf which can have all the same options but they only apply
during recover". That's much clearer than "we have two configuration

But it only means that for *some* options. Which certainly doesn't
reduce confusion.

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

On Fri, Sep 9, 2011 at 8:27 PM, Magnus Hagander <magnus@hagander.net> wrote:

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.

Agreed. After a little thought, it's came to mind that we might be
able to simply
fix that problem by making the postmaster instead of the startup process read
recovery.conf; The postmaster reads recovery.conf before it invokes the startup
process, and sets the global variables of GUC to the specified values. The child
processes including the startup process inherit the global variables of GUC, so
SHOW can return the exact setting value and the startup process can use them
to do a recovery. Even in EXEC_BACKEND case, we can adopt this idea because
those global variables are inherited from parent to children via the
save/restore_backend_variables functions.

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Sat, Sep 10, 2011 at 12:18 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

On Fri, Sep 9, 2011 at 3:05 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Magnus Hagander <magnus@hagander.net> writes:

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.

+1.  If it's not a configuration file anymore, it shouldn't be called
one.

+1 to rename file

+1 to overall concept, just thinking same myself, not looked at patch yet

Are you still thinking the backward-compatibility (i.e., the
capability to specify
recovery parameters in recovery.conf) is required?

Even if we maintain the backward-compatibility, if we rename the file, ISTM
that the tools which depends on recovery.conf would need to be changed so
that <file-with-new-name> is used instead of recovery.conf. So I'm not sure
whether leaving the capability to set parameters in recovery.conf as it is is
really worth supporting or not. If we would like to make our effort for the
backward-compatibility more worthwhile, we might have to make the path of
the file configurable as a user can set it to "recovery.conf".

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On tis, 2011-09-13 at 14:46 +0900, Fujii Masao wrote:

Are you still thinking the backward-compatibility (i.e., the
capability to specify recovery parameters in recovery.conf) is
required?

I think parameters related to a particular recovery, e.g.,
recovery_target_time, fit better into a recovery.conf that is renamed
after the recovery is complete. That was the original idea, after all.

Everything that is a permanent setting across multiple recovery
attempts, and anything related to replication, better fits elsewhere.

On Tue, Sep 13, 2011 at 2:51 PM, Peter Eisentraut <peter_e@gmx.net> wrote:

On tis, 2011-09-13 at 14:46 +0900, Fujii Masao wrote:

Are you still thinking the backward-compatibility (i.e., the
capability to specify recovery parameters in recovery.conf) is
required?

I think parameters related to a particular recovery, e.g.,
recovery_target_time, fit better into a recovery.conf that is renamed
after the recovery is complete.  That was the original idea, after all.

Everything that is a permanent setting across multiple recovery
attempts, and anything related to replication, better fits elsewhere.

I've just been thinking that a better way would be to make
recovery.conf an extension of postgresql.conf when we are in archive
recovery.

So treat postgresql.conf as if it has an automatic "include
recovery.conf" in it. The file format is the same.

That way we don't need to change existing behaviour, so any software
that relies upon this will still work, but we gain the additional
ability to reload values in recovery,conf (where appropriate).

We can change the .sample files to show parameters that make more
sense in one or the other file, rather than making it a hard
requirement for them to appear in specific files which will be a real
pain in the ass.

Internal changes would then be to move existing recovery.conf
parameters into guc.c and revise the manual accordingly.

--
 Simon Riggs                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

On Wed, Sep 14, 2011 at 1:10 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

On Tue, Sep 13, 2011 at 2:51 PM, Peter Eisentraut <peter_e@gmx.net> wrote:

On tis, 2011-09-13 at 14:46 +0900, Fujii Masao wrote:

Are you still thinking the backward-compatibility (i.e., the
capability to specify recovery parameters in recovery.conf) is
required?

I think parameters related to a particular recovery, e.g.,
recovery_target_time, fit better into a recovery.conf that is renamed
after the recovery is complete.  That was the original idea, after all.

Everything that is a permanent setting across multiple recovery
attempts, and anything related to replication, better fits elsewhere.

I've just been thinking that a better way would be to make
recovery.conf an extension of postgresql.conf when we are in archive
recovery.

So treat postgresql.conf as if it has an automatic "include
recovery.conf" in it. The file format is the same.

That way we don't need to change existing behaviour, so any software
that relies upon this will still work, but we gain the additional
ability to reload values in recovery,conf (where appropriate).

We can change the .sample files to show parameters that make more
sense in one or the other file, rather than making it a hard
requirement for them to appear in specific files which will be a real
pain in the ass.

Internal changes would then be to move existing recovery.conf
parameters into guc.c and revise the manual accordingly.

Sounds reasonable. I'll revise the patch that way.

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On tis, 2011-09-13 at 17:10 +0100, Simon Riggs wrote:

So treat postgresql.conf as if it has an automatic "include
recovery.conf" in it. The file format is the same.

Sounds good. That would also have the merit that you could use, say,
different memory settings during recovery.

On Wed, Sep 14, 2011 at 6:33 PM, Peter Eisentraut <peter_e@gmx.net> wrote:

On tis, 2011-09-13 at 17:10 +0100, Simon Riggs wrote:

So treat postgresql.conf as if it has an automatic "include
recovery.conf" in it. The file format is the same.

Sounds good.  That would also have the merit that you could use, say,
different memory settings during recovery.

One problem of this is that recovery.conf is relative to the configuration
file directory instead of data directory if we treat it as an "include" file.
If your configuration file directory is the same as the data directory, you
don't need to worry about this problem. But if you set data_directory to
the directory other than ConfigDir, the problem might break your tool
which depends on recovery.conf under the data directory.

If we'd like to treat recovery.conf as if it's under the data directory, I'm
afraid that we should add complicated code to parse recovery.conf after
the value of data_directory has been determined from postgresql.conf.
Furthermore, what if recovery.conf has another setting of data_directory?

Since recovery.conf is a configuration file, it's intuitive for me to put it
in configuration file directory rather than data directory. So I'm not inclined
to treat recovery.conf as if it's under data directory. Is this OK?

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Fujii Masao <masao.fujii@gmail.com> writes:

If we'd like to treat recovery.conf as if it's under the data directory, I'm
afraid that we should add complicated code to parse recovery.conf after
the value of data_directory has been determined from postgresql.conf.
Furthermore, what if recovery.conf has another setting of data_directory?

We already do this dance for custom_variable_classes. IIRC the only
thing you have to care about is setting the GUC before any other one. I
guess you could just process the data_directory the same way, before the
main loop, and be done with it.

Since recovery.conf is a configuration file, it's intuitive for me to put it
in configuration file directory rather than data directory. So I'm not inclined
to treat recovery.conf as if it's under data directory. Is this OK?

I think that if we keep some compatibility with recovery.conf, then we
need to include finding it in the data_directory or the configuration
file directory, in that order, and with a LOG message that says where we
did find it.

I think we should *NOT* load both of them with some priority rules.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

On tor, 2011-09-15 at 16:54 +0900, Fujii Masao wrote:

On Wed, Sep 14, 2011 at 6:33 PM, Peter Eisentraut <peter_e@gmx.net> wrote:

On tis, 2011-09-13 at 17:10 +0100, Simon Riggs wrote:

So treat postgresql.conf as if it has an automatic "include
recovery.conf" in it. The file format is the same.

Sounds good. That would also have the merit that you could use, say,
different memory settings during recovery.

One problem of this is that recovery.conf is relative to the configuration
file directory instead of data directory if we treat it as an "include" file.

Treat it *as if* it were an included file. A special included file if
you will.

If we'd like to treat recovery.conf as if it's under the data directory, I'm
afraid that we should add complicated code to parse recovery.conf after
the value of data_directory has been determined from postgresql.conf.
Furthermore, what if recovery.conf has another setting of data_directory?

Perhaps that could be addresses by inventing a new context setting
PGC_RECOVERY so that you can only set sane values.

Since recovery.conf is a configuration file, it's intuitive for me to put it
in configuration file directory rather than data directory. So I'm not inclined
to treat recovery.conf as if it's under data directory. Is this OK?

It's not a configuration file, because it disappears after use. (And a
lot of configuration file management systems would be really upset if we
had a configuration file that behaved that way.) The whole point of
this exercise is allowing the permanent configuration file parameters to
be moved to a real configuration file (postgresql.conf), while leaving
the temporary settings separately.

Alternatively, we could just forget about the whole thing and move
everything to postgresql.conf and treat recovery.conf as a simple empty
signal file. I don't know if that's necessarily better.

On Thu, Sep 15, 2011 at 5:58 AM, Peter Eisentraut <peter_e@gmx.net> wrote:

Alternatively, we could just forget about the whole thing and move
everything to postgresql.conf and treat recovery.conf as a simple empty
signal file.  I don't know if that's necessarily better.

Seems like it might be simpler.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Sep 15, 2011 at 8:04 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Sep 15, 2011 at 5:58 AM, Peter Eisentraut <peter_e@gmx.net> wrote:

Alternatively, we could just forget about the whole thing and move
everything to postgresql.conf and treat recovery.conf as a simple empty
signal file.  I don't know if that's necessarily better.

Seems like it might be simpler.

It seems to need a bit more time until we've reached a consensus about
the treatment of recovery.conf. How about committing the core patch
first, and addressing the recovery.conf issue as a different patch later?

The attached patch provides a core part of the feature, i.e., it moves
every recovery parameters from recovery.conf to postgresql.conf.
Even if you create recovery.conf, the server doesn't read it automatically.

The patch renames recovery.conf to recovery.ready, so if you want to
enter archive recovery or standby mode, you need to create
recovery.ready file in the cluster data directory. Since recovery.ready is
just a signal file, its contents have no effect.

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center