Re: Changing recovery.conf parameters into GUCs

What we want to do is make recovery parameters into GUCs, allowing
them to be reset by SIGHUP and also to allow all users to see the
parameters in use, from any session.

The existing mechanism for recovery is that
1. we put parameters in a file called recovery.conf
2. we use the existence of a recovery.conf file to trigger archive
recovery/replication

I also wish to see backwards compatibility maintained, so am proposing
the following:

a) recovery parameters are made into GUCs (for which we have a patch
from Fujii)
b) all processes automatically read recovery.conf as the last step in
reading configuration files, if it exists, even if data_directory
parameter is in use (attached patch)
c) we trigger archive recovery by the presence of either
recovery.conf or recovery.trigger in the data directory. At the end,
we rename to recovery.done just as we do now. This means that any
parameters put into recovery.conf will not be re-read when we SIGHUP
after end of recovery. Note that recovery.trigger will NOT be read for
parameters and is assumed to be zero-length.
(minor patch required)

This allows these use cases

1. Existing users create $PGDATA/recovery.conf and everything works as
before. No servers crash, because the HA instructions in the wiki
still work. Users can now see the parameters in pg_settings and we can
use SIGHUP without restarting server. Same stuff, new benefits.

2. New users wish to move their existing recovery.conf file to the
config directory. Same file contents, same file name (if desired),
same behaviour, just more convenient location for config management
tools. Recovery is triggered by recovery.trigger in $PGDATA. Trigger
and configuration are now separate, if desired.

3. Split mode. We can put things like trigger_file into the
postgresql.conf directory and also put other parameters (for example
PITR settings) into recovery.conf. Where multiple tools are in use, we
support both APIs.

Specific details...

* primary_conninfo, trigger_file and standby_mode are added to
postgresql.conf.sample
* all ex-recovery.conf parameters are SIGHUP, so no errors if
recovery.conf has changed to .done

If desired, this behaviour could be enabled by a parameter called
recovery_conf_enabled = on (default). When set to off, this would
throw an error if recovery.conf exists. (I don't see a need for this)

The patch to implement this is very small (attached). This works
standalone, but obviously barfs at the actual parameter parsing stage.
Just in case it wasn't clear, this patch is intended to go with the
parts of Fujji's patch that relate to GUC changes.

If we agree, I will merge and re-post before commit.

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

Hi,

The main argument on which this proposal is based on is to keep
backward-compatibility.
This has been discussed before many times and the position of each people
is well-known,
so I am not going back to that...

So, based on *only* what I see in this thread...

On Fri, Mar 29, 2013 at 12:48 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:

> What we want to do is make recovery parameters into GUCs, allowing
> them to be reset by SIGHUP and also to allow all users to see the
> parameters in use, from any session.
>
> The existing mechanism for recovery is that
> 1. we put parameters in a file called recovery.conf
> 2. we use the existence of a recovery.conf file to trigger archive
> recovery/replication
>
> I also wish to see backwards compatibility maintained, so am proposing
> the following:
>
> a) recovery parameters are made into GUCs (for which we have a patch
> from Fujii)
>
b) all processes automatically read recovery.conf as the last step in
> reading configuration files, if it exists, even if data_directory
> parameter is in use (attached patch)
> c) we trigger archive recovery by the presence of either
> recovery.conf or recovery.trigger in the data directory. At the end,
> we rename to recovery.done just as we do now. This means that any
> parameters put into recovery.conf will not be re-read when we SIGHUP
> after end of recovery. Note that recovery.trigger will NOT be read for
> parameters and is assumed to be zero-length.
> (minor patch required)
>
> This allows these use cases
>
> 1. Existing users create $PGDATA/recovery.conf and everything works as
> before. No servers crash, because the HA instructions in the wiki
> still work. Users can now see the parameters in pg_settings and we can
> use SIGHUP without restarting server. Same stuff, new benefits.
>
Forcing hardcoding of "include_if_exists recovery.conf" at the bottom of
postgresql.conf
is not a good thing for the user as it makes postgresql.conf processing
more opaque and
complicates parameter reloading. IMO, simplicity and transparency of
operations are
important when processing parameters.

2. New users wish to move their existing recovery.conf file to the
> config directory. Same file contents, same file name (if desired),
> same behaviour, just more convenient location for config management
> tools. Recovery is triggered by recovery.trigger in $PGDATA. Trigger
> and configuration are now separate, if desired.
>
So, people could be able to also define a recovery.trigger file? What about
the case where both recovery.conf and recovery.trigger are found in the
base folder?
Priority needs to be given to one way of processing or the other. Is it
really our goal
to confuse the user with internal priority mechanisms at least for GUC
handling?

3. Split mode. We can put things like trigger_file into the
> postgresql.conf directory and also put other parameters (for example
> PITR settings) into recovery.conf. Where multiple tools are in use, we
> support both APIs.
>
> Specific details...
>
> * primary_conninfo, trigger_file and standby_mode are added to
> postgresql.conf.sample
>
Not adding all the recovery_target_* parameters would confuse the user.
With this proposal it is actually possible to define a recovery target
inside
postgresql.conf even if the parameter is not plainly written in
postgresql.conf.sample.

> * all ex-recovery.conf parameters are SIGHUP, so no errors if
> recovery.conf has changed to .done
>
Drop of recovery.trigger at the same time? And what about the case
where both files are specified, that the server can only remove one of the
trigger files, and is then restarted with only 1 trigger file present?

If desired, this behaviour could be enabled by a parameter called
> recovery_conf_enabled = on (default). When set to off, this would
> throw an error if recovery.conf exists. (I don't see a need for this)
>
Me neither, the less GUCs the better.

> The patch to implement this is very small (attached). This works
> standalone, but obviously barfs at the actual parameter parsing stage.
> Just in case it wasn't clear, this patch is intended to go with the
> parts of Fujji's patch that relate to GUC changes.
>
> If we agree, I will merge and re-post before commit.
>
If an agreement is reached based on this proposal, I highly recommend that
you use one of the latest updated version I sent. Fujii's version had some
bugs, one of them being that as standbyModeRequested can be set to true if
specified in postgresql.conf, a portion of the code using in xlog.c can be
triggered even if ArchiveRecoveryRequested is not set to true. I also added
fixes related to documentation.

Comments from others are welcome.
--
Michael

On 29 March 2013 01:17, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:

> The main argument on which this proposal is based on is to keep
> backward-compatibility.

The main objective is to get recovery parameters as GUCs, as I said....

> On Fri, Mar 29, 2013 at 12:48 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> What we want to do is make recovery parameters into GUCs, allowing
>> them to be reset by SIGHUP and also to allow all users to see the
>> parameters in use, from any session.

From the user's perspective, we are making no changes. All recovery
parameters will work exactly the same as they always did, just now we
get to see their values more easily and we can potentially place them
in a different file if we wish. The user will have no idea that we
plan to do some internal refactoring of how we process the parameters.
So IMHO simplicity means continuing to work the way it always did
work. We simply announce "PostgreSQL now supports configuration
directories. All parameters, including recovery parameters, can be
placed in any configuration file, or in $PGDATA/recovery.conf, as
before".

We introduced "pg_ctl promote" with a new API without removing
existing ones, and some people are still in favour of keeping both
APIs. Doing the same thing here makes sense and reduces conceptual
change.

Early discussions had difficulties because of the lack of config
directories, include_if_exists and this patch. We now have the
technical capability to meet every request. Circumstances have changed
and outcomes may change also.

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

On Fri, Mar 29, 2013 at 9:59 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:

> On 29 March 2013 01:17, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:
> > On Fri, Mar 29, 2013 at 12:48 AM, Simon Riggs <simon(at)2ndquadrant(dot)com>
> wrote:
> Early discussions had difficulties because of the lack of config
> directories, include_if_exists and this patch. We now have the
> technical capability to meet every request. Circumstances have changed
> and outcomes may change also.
>
Thanks for the clarifications. The following questions are still unanswered:
1) If recovery.trigger and recovery.conf are specified. To which one the
priority is given?
2) If both recovery.trigger and recovery.conf are used, let's imagine that
the server removes recovery.trigger but fails in renaming recovery.conf but
a reason or another. Isn't there a risk of inconsistency if both triggering
methods are used at the same time?
3) Forcing a harcode of include_is_exists = 'recovery.conf' at the bottom
of postgresql.conf doesn't look like a hack?
4) Based on your proposal, are all the parameters included in
postgresql.conf.sample or not? Or only primary_conninfo, trigger_file and
standby_mode?
--
Michael

On 29 March 2013 13:24, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:
> On Fri, Mar 29, 2013 at 9:59 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>>
>> On 29 March 2013 01:17, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:
>> > On Fri, Mar 29, 2013 at 12:48 AM, Simon Riggs <simon(at)2ndquadrant(dot)com>
>> > wrote:
>> Early discussions had difficulties because of the lack of config
>> directories, include_if_exists and this patch. We now have the
>> technical capability to meet every request. Circumstances have changed
>> and outcomes may change also.
>
> Thanks for the clarifications. The following questions are still unanswered:

Minor points only. We can implement this differently if you have
alternate proposals.

> 1) If recovery.trigger and recovery.conf are specified. To which one the
> priority is given?

Neither. No priority is required. If either is present we are triggered.

> 2) If both recovery.trigger and recovery.conf are used, let's imagine that
> the server removes recovery.trigger but fails in renaming recovery.conf but
> a reason or another. Isn't there a risk of inconsistency if both triggering
> methods are used at the same time?

No. If writes to the filesystem fail, you have much bigger problems.

> 3) Forcing a harcode of include_is_exists = 'recovery.conf' at the bottom of
> postgresql.conf doesn't look like a hack?

Well, that's just an emotive term to describe something you don't
like. There are no significant downsides, just a few lines of code,
like we have in many places for various purposes, such as the support
of multiple APIs for triggering standbys.

> 4) Based on your proposal, are all the parameters included in
> postgresql.conf.sample or not? Or only primary_conninfo, trigger_file and
> standby_mode?

Other values are specific to particular situations (e.g. PITR) and if
set in the wrong context could easily break replication. We could add
them if people wish it, but it would be commented out with a clear
"don't touch these" message, so it seems more sensible to avoid them
IMHO.

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

On Fri, Mar 29, 2013 at 01:56:50PM +0000, Simon Riggs wrote:
> On 29 March 2013 13:24, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:
> > On Fri, Mar 29, 2013 at 9:59 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> >>
> >> On 29 March 2013 01:17, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:
> >> > On Fri, Mar 29, 2013 at 12:48 AM, Simon Riggs <simon(at)2ndquadrant(dot)com>
> >> > wrote:
> >> Early discussions had difficulties because of the lack of config
> >> directories, include_if_exists and this patch. We now have the
> >> technical capability to meet every request. Circumstances have changed
> >> and outcomes may change also.
> >
> > Thanks for the clarifications. The following questions are still unanswered:
>
> Minor points only. We can implement this differently if you have
> alternate proposals.

Seems we are doing design on this long after the final 9.3 commit fest
should have closed. I think we need to punt this to 9.4.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On 29 March 2013 01:17, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:

> I highly recommend that
> you use one of the latest updated version I sent. Fujii's version had some
> bugs, one of them being that as standbyModeRequested can be set to true if
> specified in postgresql.conf, a portion of the code using in xlog.c can be
> triggered even if ArchiveRecoveryRequested is not set to true. I also added
> fixes related to documentation.

Yes, that is what I meant by "Fujii's patch". He would still be
credited first, having done the main part of the work. I'll call it
Michael's updated version to avoid any further confusion.

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

Simon, All,

The new approach seems fine to me; I haven't looked at the code. If Tom
doesn't feel like it's overly complicated, then this seems like a good
compromise.

The desire to move recovery.conf/trigger to a different directory is
definitely wanted by our Debian contingent. Right now, the fact that
Debian has all .confs in /etc/, but that it doesn't work to relocate
recovery.conf, is a constant source of irritation.

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

Josh Berkus <josh(at)agliodbs(dot)com> writes:
> The desire to move recovery.conf/trigger to a different directory is
> definitely wanted by our Debian contingent. Right now, the fact that
> Debian has all .confs in /etc/, but that it doesn't work to relocate
> recovery.conf, is a constant source of irritation.

It seems like this is confusing two different problems.

If we get rid of recovery.conf per se in favor of folding the settings
into GUCs in the regular config file, then the first aspect of the issue
goes away, no? The second aspect is where to put the trigger file, and
I'm not at all convinced that anybody would want the trigger file to be
in the same place they put external config files, mainly because the
trigger has to be in a server-writable directory.

regards, tom lane

On Thu, Mar 28, 2013 at 11:48 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> What we want to do is make recovery parameters into GUCs, allowing
> them to be reset by SIGHUP and also to allow all users to see the
> parameters in use, from any session.
>
> The existing mechanism for recovery is that
> 1. we put parameters in a file called recovery.conf
> 2. we use the existence of a recovery.conf file to trigger archive
> recovery/replication
>
> I also wish to see backwards compatibility maintained, so am proposing
> the following:
>
> a) recovery parameters are made into GUCs (for which we have a patch
> from Fujii)
> b) all processes automatically read recovery.conf as the last step in
> reading configuration files, if it exists, even if data_directory
> parameter is in use (attached patch)
> c) we trigger archive recovery by the presence of either
> recovery.conf or recovery.trigger in the data directory. At the end,
> we rename to recovery.done just as we do now. This means that any
> parameters put into recovery.conf will not be re-read when we SIGHUP
> after end of recovery. Note that recovery.trigger will NOT be read for
> parameters and is assumed to be zero-length.
> (minor patch required)

I still prefer Greg Smith's proposal.

http://www.postgresql.org/message-id/4EE91248.8010505@2ndQuadrant.com

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

Robert, Simon, All,

On 04/01/2013 04:51 AM, Robert Haas wrote:> On Thu, Mar 28, 2013 at
11:48 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> a) recovery parameters are made into GUCs (for which we have a patch
>> from Fujii)
>> b) all processes automatically read recovery.conf as the last step in
>> reading configuration files, if it exists, even if data_directory
>> parameter is in use (attached patch)
>> c) we trigger archive recovery by the presence of either
>> recovery.conf or recovery.trigger in the data directory. At the end,
>> we rename to recovery.done just as we do now. This means that any
>> parameters put into recovery.conf will not be re-read when we SIGHUP
>> after end of recovery. Note that recovery.trigger will NOT be read for
>> parameters and is assumed to be zero-length.
>> (minor patch required)
>
> I still prefer Greg Smith's proposal.
>
> http://www.postgresql.org/message-id/4EE91248.8010505@2ndQuadrant.com

So, this seems to still be stalled. Is there a way forward on this
which won't cause us to wait *another* version before we have working
replication GUCs?

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

On Sat, Jul 6, 2013 at 3:49 AM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> Robert, Simon, All,
>
> On 04/01/2013 04:51 AM, Robert Haas wrote:> On Thu, Mar 28, 2013 at
> 11:48 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>>> a) recovery parameters are made into GUCs (for which we have a patch
>>> from Fujii)
>>> b) all processes automatically read recovery.conf as the last step in
>>> reading configuration files, if it exists, even if data_directory
>>> parameter is in use (attached patch)
>>> c) we trigger archive recovery by the presence of either
>>> recovery.conf or recovery.trigger in the data directory. At the end,
>>> we rename to recovery.done just as we do now. This means that any
>>> parameters put into recovery.conf will not be re-read when we SIGHUP
>>> after end of recovery. Note that recovery.trigger will NOT be read for
>>> parameters and is assumed to be zero-length.
>>> (minor patch required)
>>
>> I still prefer Greg Smith's proposal.
>>
>> http://www.postgresql.org/message-id/4EE91248.8010505@2ndQuadrant.com
>
> So, this seems to still be stalled. Is there a way forward on this
> which won't cause us to wait *another* version before we have working
> replication GUCs?
Yeah, it would be good to revive this thread now, which is the
beginning of the development cycle. As of now, just to recall
everybody, an agreement on this patch still needs to be found... Simon
is concerned with backward compatibility. Greg presented a nice spec
some time ago (Robert and I liked it) which would break backward
compatibility but allow to begin with a fresh infrastructure.
--
Michael

On 07/05/2013 10:09 PM, Michael Paquier wrote:
> Yeah, it would be good to revive this thread now, which is the
> beginning of the development cycle. As of now, just to recall
> everybody, an agreement on this patch still needs to be found... Simon
> is concerned with backward compatibility. Greg presented a nice spec
> some time ago (Robert and I liked it) which would break backward
> compatibility but allow to begin with a fresh infrastructure.

As folks know, I favor Smith's approach. However, as far as I can find,
GSmith never posted a patch for his version.

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

On 2013/07/09, at 4:09, Josh Berkus <josh(at)agliodbs(dot)com> wrote:

> On 07/05/2013 10:09 PM, Michael Paquier wrote:
>> Yeah, it would be good to revive this thread now, which is the
>> beginning of the development cycle. As of now, just to recall
>> everybody, an agreement on this patch still needs to be found... Simon
>> is concerned with backward compatibility. Greg presented a nice spec
>> some time ago (Robert and I liked it) which would break backward
>> compatibility but allow to begin with a fresh infrastructure.
>
> As folks know, I favor Smith's approach. However, as far as I can find,
> GSmith never posted a patch for his version.
Actually I did.
http://www.postgresql.org/message-id/CAB7nPqR+fpopEDMoecK+AfZB5a8kUUvxpU=1a2JiX5d9s=0s6Q@mail.gmail.com
--
Michael
(Sent from my mobile phone)

On 5 July 2013 19:49, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> Robert, Simon, All,
>
> On 04/01/2013 04:51 AM, Robert Haas wrote:> On Thu, Mar 28, 2013 at
> 11:48 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>>> a) recovery parameters are made into GUCs (for which we have a patch
>>> from Fujii)
>>> b) all processes automatically read recovery.conf as the last step in
>>> reading configuration files, if it exists, even if data_directory
>>> parameter is in use (attached patch)
>>> c) we trigger archive recovery by the presence of either
>>> recovery.conf or recovery.trigger in the data directory. At the end,
>>> we rename to recovery.done just as we do now. This means that any
>>> parameters put into recovery.conf will not be re-read when we SIGHUP
>>> after end of recovery. Note that recovery.trigger will NOT be read for
>>> parameters and is assumed to be zero-length.
>>> (minor patch required)
>>
>> I still prefer Greg Smith's proposal.
>>
>> http://www.postgresql.org/message-id/4EE91248.8010505@2ndQuadrant.com
>
> So, this seems to still be stalled. Is there a way forward on this
> which won't cause us to wait *another* version before we have working
> replication GUCs?

This needs to be broken down rather than just say "I like Greg's
proposal", or I have written a patch. Writing the patch is not the/an
issue.

Greg's points were these (I have numbered them and named/characterised them)

1. MOVE SETTINGS
All settings move into the postgresql.conf.

Comment: AFAIK, all agree this.

2. RELOCATE RECOVERY PARAMETER FILE(s)
As of 9.2, relocating the postgresql.conf means there are no user
writable files needed in the data directory.

Comment: AFAIK, all except Heikki wanted this. He has very strong
objections to my commit that would have allowed relocating
recovery.conf outside of the data directory. By which he means both
the concepts of triggerring replication and of specifying parameters.
Changes in 9.3 specifically write files to the data directory that
expect this.

3. SEPARATE TRIGGER FILE
Creating a standby.enabled file in the directory that houses the
postgresql.conf (same logic as "include" uses to locate things) puts the
system into recovery mode. That feature needs to save some state, and
making those decisions based on existence of a file is already a thing
we do. Rather than emulating the rename to recovery.done that happens
now, the server can just delete it, to keep from incorrectly returning
to a state it's exited. A UI along the lines of the promote one,
allowing "pg_ctl standby", should fall out of here. I think this is
enough that people who just want to use replication features need never
hear about this file at all, at least during the important to simplify
first run through.

Comment: AFAIK, all except Heikki are OK with this.

4. DISALLOW PREVIOUS API
If startup finds a recovery.conf file where it used to live at,
abort--someone is expecting the old behavior. Hint to RTFM or include a
short migration guide right on the spot. That can have a nice section
about how you might use the various postgresql.conf include* features if
they want to continue managing those files separately. Example: rename
it as replication.conf and use include_if_exists if you want to be able
to rename it to recovery.done like before. Or drop it into a conf.d
directory where the rename will make it then skipped.

Comment: I am against this. Tool vendors are not the problem here.
There is no good reason to just break everybody's scripts with no
warning of future deprecataion and an alternate API, especially since
we now allow multiple parameter files.

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

On 07/08/2013 11:43 PM, Simon Riggs wrote:
> This needs to be broken down rather than just say "I like Greg's
> proposal", or I have written a patch. Writing the patch is not the/an
> issue.
>
> Greg's points were these (I have numbered them and named/characterised them)

Thanks for the nice summary! Makes it easy for the rest of us to address.

>
> 1. MOVE SETTINGS
> All settings move into the postgresql.conf.
>
> Comment: AFAIK, all agree this.

Good to go then.

> 2. RELOCATE RECOVERY PARAMETER FILE(s)
> As of 9.2, relocating the postgresql.conf means there are no user
> writable files needed in the data directory.
>
> Comment: AFAIK, all except Heikki wanted this. He has very strong
> objections to my commit that would have allowed relocating
> recovery.conf outside of the data directory. By which he means both
> the concepts of triggerring replication and of specifying parameters.
> Changes in 9.3 specifically write files to the data directory that
> expect this.

Yeah, I didn't understand why this was shot down either.

Heikki?

> 3. SEPARATE TRIGGER FILE
> Creating a standby.enabled file in the directory that houses the
> postgresql.conf (same logic as "include" uses to locate things) puts the
> system into recovery mode. That feature needs to save some state, and
> making those decisions based on existence of a file is already a thing
> we do. Rather than emulating the rename to recovery.done that happens
> now, the server can just delete it, to keep from incorrectly returning
> to a state it's exited. A UI along the lines of the promote one,
> allowing "pg_ctl standby", should fall out of here. I think this is
> enough that people who just want to use replication features need never
> hear about this file at all, at least during the important to simplify
> first run through.
>
> Comment: AFAIK, all except Heikki are OK with this.

One bit of complexity I'd like to see go away is that we have two
trigger files: one to put a server into replication, and one to promote
it. The promotion trigger file is a legacy of warm standby, I believe.
Maybe, now that we have pg_ctl promote available, we can eliminate the
promotion trigger?

Also, previously, deleting the recovery.conf file did not cause the
server to be promoted AFAIK. Is that something we should change if
we're going to keep a trigger file to start replication?

Also, I'm not keen on the idea that the start-replication trigger file
will still be *required*. I really want to be able to manage my setup
entirely through configuration/pg_ctl directives and not be forced to
use a trigger file.

> 4. DISALLOW PREVIOUS API
> If startup finds a recovery.conf file where it used to live at,
> abort--someone is expecting the old behavior. Hint to RTFM or include a
> short migration guide right on the spot. That can have a nice section
> about how you might use the various postgresql.conf include* features if
> they want to continue managing those files separately. Example: rename
> it as replication.conf and use include_if_exists if you want to be able
> to rename it to recovery.done like before. Or drop it into a conf.d
> directory where the rename will make it then skipped.
>
> Comment: I am against this. Tool vendors are not the problem here.
> There is no good reason to just break everybody's scripts with no
> warning of future deprecataion and an alternate API, especially since
> we now allow multiple parameter files.

Well, the issue is not so much the presence of a recovery.conf file full
of config variables ... which as you point out is now effectively
supported ... but the use of that as a trigger file. So I think the
two points you make here are flipped.

Personally, I don't care if we support the old recovery.conf-trigger
behavior as long as I'm not forced to use it. The main objection to
supporting both was code complexity, I believe.

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

On 10.07.2013 02:54, Josh Berkus wrote:
> On 07/08/2013 11:43 PM, Simon Riggs wrote:
>> 1. MOVE SETTINGS
>> All settings move into the postgresql.conf.
>>
>> Comment: AFAIK, all agree this.
>
> Good to go then.

+1.

>> 2. RELOCATE RECOVERY PARAMETER FILE(s)
>> As of 9.2, relocating the postgresql.conf means there are no user
>> writable files needed in the data directory.
>>
>> Comment: AFAIK, all except Heikki wanted this. He has very strong
>> objections to my commit that would have allowed relocating
>> recovery.conf outside of the data directory. By which he means both
>> the concepts of triggerring replication and of specifying parameters.
>> Changes in 9.3 specifically write files to the data directory that
>> expect this.
>
> Yeah, I didn't understand why this was shot down either.
>
> Heikki?

Does this refer to this:

http://www.postgresql.org/message-id/5152F778.2070205@vmware.com

? I listed some objections and suggestions there. Probably the biggest
issue back then, however, was that it was committed so late in the
release cycle. In any case, relocating the config/trigger file has
nothing to do with turning recovery.conf parameters into GUCs, so let's
not confuse this patch with that goal.

>> 3. SEPARATE TRIGGER FILE
>> Creating a standby.enabled file in the directory that houses the
>> postgresql.conf (same logic as "include" uses to locate things) puts the
>> system into recovery mode. That feature needs to save some state, and
>> making those decisions based on existence of a file is already a thing
>> we do. Rather than emulating the rename to recovery.done that happens
>> now, the server can just delete it, to keep from incorrectly returning
>> to a state it's exited. A UI along the lines of the promote one,
>> allowing "pg_ctl standby", should fall out of here. I think this is
>> enough that people who just want to use replication features need never
>> hear about this file at all, at least during the important to simplify
>> first run through.
>>
>> Comment: AFAIK, all except Heikki are OK with this.

Sorry, I don't quite understand what this is about. Can you point me to
the previous discussion on this?

"pg_ctl standby" sounds handy. It's not very useful without something
like pg_rewind or some other mechanism to do a clean failover, though.
Have to make sure that we have enough safeguards in place that you can't
shoot yourself in the foot with that, though; if you turn a master
server into a standby with that, must make sure that you can't corrupt
the database if you point that standby to another standby.

But I don't see how that's related to changing recovery.conf parameters
into gucs.

> One bit of complexity I'd like to see go away is that we have two
> trigger files: one to put a server into replication, and one to promote
> it. The promotion trigger file is a legacy of warm standby, I believe.
> Maybe, now that we have pg_ctl promote available, we can eliminate the
> promotion trigger?

No, see http://www.postgresql.org/message-id/5112A54B.8090500@vmware.com.

> Also, previously, deleting the recovery.conf file did not cause the
> server to be promoted AFAIK. Is that something we should change if
> we're going to keep a trigger file to start replication?

Deleting recovery.conf file (and restarting) takes the server out of
standby mode, but in an unsafe way. Yeah, would be nice to do something
about that.

>> 4. DISALLOW PREVIOUS API
>> If startup finds a recovery.conf file where it used to live at,
>> abort--someone is expecting the old behavior. Hint to RTFM or include a
>> short migration guide right on the spot. That can have a nice section
>> about how you might use the various postgresql.conf include* features if
>> they want to continue managing those files separately. Example: rename
>> it as replication.conf and use include_if_exists if you want to be able
>> to rename it to recovery.done like before. Or drop it into a conf.d
>> directory where the rename will make it then skipped.
>>
>> Comment: I am against this. Tool vendors are not the problem here.
>> There is no good reason to just break everybody's scripts with no
>> warning of future deprecataion and an alternate API, especially since
>> we now allow multiple parameter files.
>
> Well, the issue is not so much the presence of a recovery.conf file full
> of config variables ... which as you point out is now effectively
> supported ... but the use of that as a trigger file. So I think the
> two points you make here are flipped.
>
> Personally, I don't care if we support the old recovery.conf-trigger
> behavior as long as I'm not forced to use it. The main objection to
> supporting both was code complexity, I believe.

I'm also undecided on this one. If we can figure out a good way forward
that keeps backwards-compatibility, good. But it's not worth very much
to me - if we can get a better interface overall by dropping
backwards-compatibility, then let's drop it.

- Heikki

On 7/10/13 9:39 AM, Heikki Linnakangas wrote:
> On 10.07.2013 02:54, Josh Berkus wrote:
>> One bit of complexity I'd like to see go away is that we have two
>> trigger files: one to put a server into replication, and one to promote
>> it. The promotion trigger file is a legacy of warm standby, I believe.
>> Maybe, now that we have pg_ctl promote available, we can eliminate the
>> promotion trigger?
>
> No, see http://www.postgresql.org/message-id/5112A54B.8090500@vmware.com.

Right, you had some good points in there. STONITH is so hard already,
we need to be careful about eliminating options there.

All the summaries added here have actually managed to revive this one
usefully early in the release cycle! Well done. I just tried to apply
Michael's 20130325_recovery_guc_v3.patch and the bit rot isn't that bad
either. 5 rejection files, nothing big in them.

The only overlap between the recovery.conf GUC work and refactoring the
trigger file is that the trigger file is referenced in there, and we
really need to point it somewhere to be most useful.

>> Personally, I don't care if we support the old recovery.conf-trigger
>> behavior as long as I'm not forced to use it.

If you accept Heikki's argument for why the file can't go away
altogether, and it's pretty reasonable, I think two changes reach a
point where everyone can live with this:

-We need a useful default filename for trigger_file to point at.
-"pg_ctl failover" creates that file.

As for the location to default to, the pg_standby docs suggest
/tmp/pgsql.trigger.5432 while the "Binary Replication Tutorial" on the
wiki uses a RedHat directory layout $PGDATA/failover

The main reason I've preferred something in the data directory is that
triggering a standby is too catastrophic for me to be comfortable
putting it in /tmp. Any random hooligan with a shell account can
trigger a standby and break its replication. Putting the unix socket
into /tmp only works because the server creates the file as part of
startup. Here that's not possible, because creating the trigger is the
signalling mechanism.

I don't think there needs to be a CLI interface for putting the
alternate possible text into the trigger--that you can ask for 'fast'
startup. It's nice to have available as an expert, but it's fine for
that to be harder to do.

--
Greg Smith 2ndQuadrant US greg(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com

On Mon, Jul 15, 2013 at 9:09 AM, Greg Smith <greg(at)2ndquadrant(dot)com> wrote:
> All the summaries added here have actually managed to revive this one
> usefully early in the release cycle! Well done. I just tried to apply
> Michael's 20130325_recovery_guc_v3.patch and the bit rot isn't that bad
> either. 5 rejection files, nothing big in them.
Not sure if it will help, but I have in one of my dev branches a
version of this patch in sync with master branch... Please see
attached... At least it will avoid to have to realign the code of v3.
--
Michael