pgsql: Allow external recovery_config_directory

Started by Simon Riggsalmost 13 years ago45 messages
#1Simon Riggs
simon@2ndQuadrant.com

Allow external recovery_config_directory
If required, recovery.conf can now be located outside of the data directory.
Server needs read/write permissions on this directory.

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/bc5334d8679c428a709d150666b288171795bd76

Modified Files
--------------
doc/src/sgml/config.sgml | 17 +++++++++++++++++
src/backend/access/transam/xlog.c | 18 +++++++++++-------
src/backend/utils/init/globals.c | 1 +
src/backend/utils/init/miscinit.c | 19 +++++++++++++++++++
src/backend/utils/misc/guc.c | 24 ++++++++++++++++++++++++
src/include/miscadmin.h | 2 ++
src/include/utils/guc.h | 1 +
7 files changed, 75 insertions(+), 7 deletions(-)

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

#2Magnus Hagander
magnus@hagander.net
In reply to: Simon Riggs (#1)
Re: pgsql: Allow external recovery_config_directory

On Wed, Mar 27, 2013 at 12:47 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

Allow external recovery_config_directory
If required, recovery.conf can now be located outside of the data directory.
Server needs read/write permissions on this directory.

Should we consider verifying that it has write permissions and fail
early if not?

AFAICT, right now we fail *after* doing a bunch of things if the user
only has readonly permissions?

I'm sure a common thing will be for people to try to set this to
/etc/postgresql or similar...

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

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

#3Heikki Linnakangas
hlinnakangas@vmware.com
In reply to: Simon Riggs (#1)
Re: [COMMITTERS] pgsql: Allow external recovery_config_directory

On 27.03.2013 13:47, Simon Riggs wrote:

Allow external recovery_config_directory
If required, recovery.conf can now be located outside of the data directory.
Server needs read/write permissions on this directory.

This was a surprising commit. Does this get us any closer to merging
postgresql.conf and recovery.conf?

- Heikki

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#4Michael Paquier
michael.paquier@gmail.com
In reply to: Heikki Linnakangas (#3)
Re: [COMMITTERS] pgsql: Allow external recovery_config_directory

On Wed, Mar 27, 2013 at 9:12 PM, Heikki Linnakangas <hlinnakangas@vmware.com

wrote:

On 27.03.2013 13:47, Simon Riggs wrote:

Allow external recovery_config_directory
If required, recovery.conf can now be located outside of the data
directory.
Server needs read/write permissions on this directory.

This was a surprising commit. Does this get us any closer to merging
postgresql.conf and recovery.conf?

At first glance, I am not sure this goes in the right direction. Why is it
necessary to add a GUC parameter for that?
In the patch I sent based on Masao's first version, if we merge of
postgresql.conf and recovery.conf, users will be encouraged to migrate to
the new system by including recovery.conf or a file containing recovery
parameters using include_path or include_if_exists, so you shouldn't need a
new parameter to include recovery.conf. I have the feeling that this
complicates even more the settings.
Also, based on Greg's spec (that Robert and I basically agreed on), if
recovery.conf is found at the root of data folder an error is returned to
user, recommending him to migrate correctly by referring to dedicated
documentation.
--
Michael

#5Michael Paquier
michael.paquier@gmail.com
In reply to: Michael Paquier (#4)
Re: [COMMITTERS] pgsql: Allow external recovery_config_directory

On Wed, Mar 27, 2013 at 9:59 PM, Michael Paquier
<michael.paquier@gmail.com>wrote:

On Wed, Mar 27, 2013 at 9:12 PM, Heikki Linnakangas <
hlinnakangas@vmware.com> wrote:

On 27.03.2013 13:47, Simon Riggs wrote:

Allow external recovery_config_directory
If required, recovery.conf can now be located outside of the data
directory.
Server needs read/write permissions on this directory.

This was a surprising commit. Does this get us any closer to merging
postgresql.conf and recovery.conf?

At first glance, I am not sure this goes in the right direction. Why is it
necessary to add a GUC parameter for that?
In the patch I sent based on Masao's first version, if we merge of
postgresql.conf and recovery.conf, users will be encouraged to migrate to
the new system by including recovery.conf or a file containing recovery
parameters using include_path or include_if_exists, so you shouldn't need a
new parameter to include recovery.conf. I have the feeling that this
complicates even more the settings.
Also, based on Greg's spec (that Robert and I basically agreed on), if
recovery.conf is found at the root of data folder an error is returned to
user, recommending him to migrate correctly by referring to dedicated
documentation.

Please note also that based on the documentation include* params can used
absolute paths:
http://www.postgresql.org/docs/devel/static/config-setting.html#CONFIG-INCLUDES
--
Michael

#6Simon Riggs
simon@2ndQuadrant.com
In reply to: Heikki Linnakangas (#3)
Re: [COMMITTERS] pgsql: Allow external recovery_config_directory

On 27 March 2013 12:12, Heikki Linnakangas <hlinnakangas@vmware.com> wrote:

On 27.03.2013 13:47, Simon Riggs wrote:

Allow external recovery_config_directory
If required, recovery.conf can now be located outside of the data
directory.
Server needs read/write permissions on this directory.

This was a surprising commit. Does this get us any closer to merging
postgresql.conf and recovery.conf?

Why so? I made a clear proposal on how to proceed and this was the
first part of it.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#7Simon Riggs
simon@2ndQuadrant.com
In reply to: Michael Paquier (#4)
Re: [COMMITTERS] pgsql: Allow external recovery_config_directory

On 27 March 2013 12:59, Michael Paquier <michael.paquier@gmail.com> wrote:

Also, based on Greg's spec (that Robert and I basically agreed on), if
recovery.conf is found at the root of data folder an error is returned to
user, recommending him to migrate correctly by referring to dedicated
documentation.

I'm following what was agreed on 24 December.

We can have the whole debate again, if you wish. There is no reason to
break backwards compatibility to get what we want.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#8Simon Riggs
simon@2ndQuadrant.com
In reply to: Magnus Hagander (#2)
Re: pgsql: Allow external recovery_config_directory

On 27 March 2013 12:02, Magnus Hagander <magnus@hagander.net> wrote:

On Wed, Mar 27, 2013 at 12:47 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

Allow external recovery_config_directory
If required, recovery.conf can now be located outside of the data directory.
Server needs read/write permissions on this directory.

Should we consider verifying that it has write permissions and fail
early if not?

Sounds like a good plan. Will do.

AFAICT, right now we fail *after* doing a bunch of things if the user
only has readonly permissions?

I'm sure a common thing will be for people to try to set this to
/etc/postgresql or similar...

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

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

#9Thom Brown
thom@linux.com
In reply to: Simon Riggs (#1)
Re: pgsql: Allow external recovery_config_directory

On 27 March 2013 11:47, Simon Riggs <simon@2ndquadrant.com> wrote:

Allow external recovery_config_directory
If required, recovery.conf can now be located outside of the data directory.
Server needs read/write permissions on this directory.

The docs don't build due to this commit. Please remove the unpaired
"<variablelist>" from line 333 of config.sgml.

--
Thom

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

#10Robert Haas
robertmhaas@gmail.com
In reply to: Simon Riggs (#7)
Re: [COMMITTERS] pgsql: Allow external recovery_config_directory

On Wed, Mar 27, 2013 at 9:11 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

On 27 March 2013 12:59, Michael Paquier <michael.paquier@gmail.com> wrote:

Also, based on Greg's spec (that Robert and I basically agreed on), if
recovery.conf is found at the root of data folder an error is returned to
user, recommending him to migrate correctly by referring to dedicated
documentation.

I'm following what was agreed on 24 December.

I assume that you are referring to this message:

/messages/by-id/CA+U5nMK8n=sQ-xPvBVtiCS3NbVObjUVM5xBR+FAeQN-RjjGxSQ@mail.gmail.com

I don't see a followup from anyone clearly agreeing that this was a
useful thing to do. There is a lot of support for turning
recovery.conf parameters into GUCs. But I don't remember anyone
supporting this idea, and like Heikki and Michael, I don't understand
how it moves the ball forward.

Considering there's been no discussion of this particular change in
three months, and not a whole lot back then, I think it would have
been polite to post the patch and ask for comments before committing
it.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#11Simon Riggs
simon@2ndQuadrant.com
In reply to: Robert Haas (#10)
Re: [COMMITTERS] pgsql: Allow external recovery_config_directory

On 27 March 2013 13:21, Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Mar 27, 2013 at 9:11 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

On 27 March 2013 12:59, Michael Paquier <michael.paquier@gmail.com> wrote:

Also, based on Greg's spec (that Robert and I basically agreed on), if
recovery.conf is found at the root of data folder an error is returned to
user, recommending him to migrate correctly by referring to dedicated
documentation.

I'm following what was agreed on 24 December.

I assume that you are referring to this message:

/messages/by-id/CA+U5nMK8n=sQ-xPvBVtiCS3NbVObjUVM5xBR+FAeQN-RjjGxSQ@mail.gmail.com

I don't see a followup from anyone clearly agreeing that this was a
useful thing to do.

Please look again.

There is a lot of support for turning
recovery.conf parameters into GUCs.

Who is against it? I am not. Even, I am working on it now, as already
said in at least 3 different places.

But I don't remember anyone
supporting this idea, and like Heikki and Michael, I don't understand
how it moves the ball forward.

Considering there's been no discussion of this particular change in
three months, and not a whole lot back then, I think it would have
been polite to post the patch and ask for comments before committing
it.

Given various confusions and multiple patches, posting another
wouldn't help much.

In terms of politeness, I certainly mean no rudeness, only to move
forward as agreed.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#12Simon Riggs
simon@2ndQuadrant.com
In reply to: Thom Brown (#9)
Re: pgsql: Allow external recovery_config_directory

On 27 March 2013 13:14, Thom Brown <thom@linux.com> wrote:

On 27 March 2013 11:47, Simon Riggs <simon@2ndquadrant.com> wrote:

Allow external recovery_config_directory
If required, recovery.conf can now be located outside of the data directory.
Server needs read/write permissions on this directory.

The docs don't build due to this commit. Please remove the unpaired
"<variablelist>" from line 333 of config.sgml.

Thanks

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

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

#13Heikki Linnakangas
hlinnakangas@vmware.com
In reply to: Simon Riggs (#7)
Re: [COMMITTERS] pgsql: Allow external recovery_config_directory

On 27.03.2013 15:11, Simon Riggs wrote:

On 27 March 2013 12:59, Michael Paquier<michael.paquier@gmail.com> wrote:

Also, based on Greg's spec (that Robert and I basically agreed on), if
recovery.conf is found at the root of data folder an error is returned to
user, recommending him to migrate correctly by referring to dedicated
documentation.

I'm following what was agreed on 24 December.

Well, there wasn't much discussion about it back then. The way I read
the thread is that people agreed with the general approach, as now
implemented in Michael's patch, based on Fujii's earlier patch. This
might be a good idea or not, but it's a new and separate feature, not
related to whatever else we might do with recovery.conf.

If we are to discuss the merits of this patch now, a few thoughts:

1. This is going to make life more complicated for tools that want to
mess with recovery.conf, as it's no longer guaranteed to be in $PGDATA.

2. An admin can no longer tell if a server is in standby or PITR mode
just by checking for $PGDATA/recovery.conf

3. Would it make sense to make the option "recovery_config_file",
pointing to the file, instead of just the directory?

4. Could you achieve the same with a symlink in $PGDATA?

We can have the whole debate again, if you wish. There is no reason to
break backwards compatibility to get what we want.

AFAICS this is completely orthogonal to backwards-compatibility and
other aspects of the upcoming patch to merge recovery.conf and
postgresql.conf.

- Heikki

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#14Robert Haas
robertmhaas@gmail.com
In reply to: Simon Riggs (#11)
Re: [COMMITTERS] pgsql: Allow external recovery_config_directory

On Wed, Mar 27, 2013 at 9:40 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

On 27 March 2013 13:21, Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Mar 27, 2013 at 9:11 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

On 27 March 2013 12:59, Michael Paquier <michael.paquier@gmail.com> wrote:

Also, based on Greg's spec (that Robert and I basically agreed on), if
recovery.conf is found at the root of data folder an error is returned to
user, recommending him to migrate correctly by referring to dedicated
documentation.

I'm following what was agreed on 24 December.

I assume that you are referring to this message:

/messages/by-id/CA+U5nMK8n=sQ-xPvBVtiCS3NbVObjUVM5xBR+FAeQN-RjjGxSQ@mail.gmail.com

I don't see a followup from anyone clearly agreeing that this was a
useful thing to do.

Please look again.

I have a better idea: how about if you tell me where you see any such
message? Because I think the reason I don't see it is because it
doesn't exist.

It's not my job to go back and scour the archives for evidence that
there is some consensus around this commit. It's your job to provide
some evidence that such a consensus exists. Or else revert the
commit, because so far no one but you seems to believe that this was a
good idea. The fact that nobody specifically objected to one line in
an email message you posted three months ago does not constitute
grounds to go change it without so much as posting the patch. Or at
least I can't imagine that any other committer would take it that way.
Even Tom posts his patches before committing them, unless there's
been specific and recent discussion of the topic. What gives you the
right to do otherwise?

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#15Andrew Dunstan
andrew@dunslane.net
In reply to: Simon Riggs (#1)
Re: pgsql: Allow external recovery_config_directory

On 03/27/2013 07:47 AM, Simon Riggs wrote:

Allow external recovery_config_directory
If required, recovery.conf can now be located outside of the data directory.
Server needs read/write permissions on this directory.

This patch appears to have broken Windows builds on the buildfarm.

cheers

andrew

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

#16Simon Riggs
simon@2ndQuadrant.com
In reply to: Robert Haas (#14)
Re: [COMMITTERS] pgsql: Allow external recovery_config_directory

On 27 March 2013 13:53, Robert Haas <robertmhaas@gmail.com> wrote:

Please look again.

I have a better idea: how about if you tell me where you see any such
message? Because I think the reason I don't see it is because it
doesn't exist.

/messages/by-id/20130109204225.GB21747@momjian.us
/messages/by-id/20121224171529.GB19778@alap2.fritz.box

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#17Robert Haas
robertmhaas@gmail.com
In reply to: Simon Riggs (#16)
Re: [COMMITTERS] pgsql: Allow external recovery_config_directory

On Wed, Mar 27, 2013 at 10:06 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

On 27 March 2013 13:53, Robert Haas <robertmhaas@gmail.com> wrote:

Please look again.

I have a better idea: how about if you tell me where you see any such
message? Because I think the reason I don't see it is because it
doesn't exist.

/messages/by-id/20130109204225.GB21747@momjian.us
/messages/by-id/20121224171529.GB19778@alap2.fritz.box

I don't think the first one can be read as a message of support for
this change, but I agree that the second one can.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#18Michael Paquier
michael.paquier@gmail.com
In reply to: Simon Riggs (#7)
Re: [COMMITTERS] pgsql: Allow external recovery_config_directory

On Wed, Mar 27, 2013 at 10:11 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

On 27 March 2013 12:59, Michael Paquier <michael.paquier@gmail.com> wrote:

Also, based on Greg's spec (that Robert and I basically agreed on), if
recovery.conf is found at the root of data folder an error is returned to
user, recommending him to migrate correctly by referring to dedicated
documentation.

I'm following what was agreed on 24 December.

We can have the whole debate again, if you wish. There is no reason to
break backwards compatibility to get what we want.

OK here is an idea I just got: why not replacing the possibility to define
a custom repository for recovery.conf by the possibility to define a custom
*file*?

Here would be the plan:
1) we move all the recovery parameters to GUC as proposed Masao's patch
(and somewhat my patch now).
2) as default, the custom file that is used to trigger recovery is called
recovery.conf and needs to be located in data folder. This could be the
default value used by this feature.
3) When migrating to the new system, we recommend users to include
recovery.conf with include_if_exists. Even better, we add by default an
include_if_exists during initdb in postgresql.conf to include recovery.conf.

If we do that users don't even need to perform any migration operation.
You don't even need to maintain two parsing interfaces for recovery
parameters, and you don't even need to think about things like which file
has the priority on the other.
So happy end.
--
Michael

#19Michael Paquier
michael.paquier@gmail.com
In reply to: Heikki Linnakangas (#13)
Re: [COMMITTERS] pgsql: Allow external recovery_config_directory

On Wed, Mar 27, 2013 at 10:43 PM, Heikki Linnakangas <
hlinnakangas@vmware.com> wrote:

3. Would it make sense to make the option "recovery_config_file", pointing
to the file, instead of just the directory?

+1 on that. I just sent the same suggestion.
-- 
Michael
#20Heikki Linnakangas
hlinnakangas@vmware.com
In reply to: Simon Riggs (#6)
Re: [COMMITTERS] pgsql: Allow external recovery_config_directory

On 27.03.2013 15:09, Simon Riggs wrote:

On 27 March 2013 12:12, Heikki Linnakangas<hlinnakangas@vmware.com> wrote:

On 27.03.2013 13:47, Simon Riggs wrote:

Allow external recovery_config_directory
If required, recovery.conf can now be located outside of the data
directory.
Server needs read/write permissions on this directory.

This was a surprising commit. Does this get us any closer to merging
postgresql.conf and recovery.conf?

Why so? I made a clear proposal on how to proceed and this was the
first part of it.

You didn't answer the question. Does this get us any closer to merging
postgresql.conf and recovery.conf? Why is this bundled in with that?

ISTM the quickest way forward is to revert this, and proceed with the
rest of the plan: get Michael/Fujii's patch into shape, and commit it.
If we still think this additional GUC is a good idea after that, we can
add it afterwards just as well.

- Heikki

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#21Robert Haas
robertmhaas@gmail.com
In reply to: Michael Paquier (#18)
Re: [COMMITTERS] pgsql: Allow external recovery_config_directory

On Wed, Mar 27, 2013 at 10:15 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

Here would be the plan:
1) we move all the recovery parameters to GUC as proposed Masao's patch (and
somewhat my patch now).
2) as default, the custom file that is used to trigger recovery is called
recovery.conf and needs to be located in data folder. This could be the
default value used by this feature.
3) When migrating to the new system, we recommend users to include
recovery.conf with include_if_exists. Even better, we add by default an
include_if_exists during initdb in postgresql.conf to include recovery.conf.

I don't think it's a good to call the trigger file recovery.conf.
That's just plain confusing.

There are also weird edge cases here when the server is promoted. The
recovery.conf file won't exist any more, but the GUC settings changes
it contains will live on until the next SIGHUP.

The proposal Greg Smith floated previously, which I supported and I
believe others did as well, was to convert the parameters to GUCs, and
error out if recovery.conf existed. That way, anyone who is doing it
wrong (for the new release), gets a clear error message. If we were
doing that (and I had somehow thought THAT was the consensus), then
this patch is moot, because you can't set a location for recovery.conf
if that concept doesn't exist any more. You might need a parameter to
set the location for the trigger file, perhaps.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#22Michael Paquier
michael.paquier@gmail.com
In reply to: Robert Haas (#21)
Re: [COMMITTERS] pgsql: Allow external recovery_config_directory

On Wed, Mar 27, 2013 at 11:26 PM, Robert Haas <robertmhaas@gmail.com> wrote:

There are also weird edge cases here when the server is promoted. The
recovery.conf file won't exist any more, but the GUC settings changes
it contains will live on until the next SIGHUP.

Yes indeed, I forgot that.

The proposal Greg Smith floated previously, which I supported and I

believe others did as well, was to convert the parameters to GUCs, and
error out if recovery.conf existed. That way, anyone who is doing it
wrong (for the new release), gets a clear error message. If we were
doing that (and I had somehow thought THAT was the consensus), then
this patch is moot, because you can't set a location for recovery.conf
if that concept doesn't exist any more. You might need a parameter to
set the location for the trigger file, perhaps.

I am perfectly fine with this plan, especially the part where server errors
out if recovery.conf is found. It makes clear to the user that it is not
supported
anymore.

I just tried to find some new fresh ideas that would satisfy everybody... So
let's keep up with the plan that Greg proposed and forget what I wrote
previously.
--
Michael

#23Simon Riggs
simon@2ndQuadrant.com
In reply to: Heikki Linnakangas (#20)
Re: [COMMITTERS] pgsql: Allow external recovery_config_directory

On 27 March 2013 14:20, Heikki Linnakangas <hlinnakangas@vmware.com> wrote:

On 27.03.2013 15:09, Simon Riggs wrote:

On 27 March 2013 12:12, Heikki Linnakangas<hlinnakangas@vmware.com>
wrote:

On 27.03.2013 13:47, Simon Riggs wrote:

Allow external recovery_config_directory
If required, recovery.conf can now be located outside of the data
directory.
Server needs read/write permissions on this directory.

This was a surprising commit. Does this get us any closer to merging
postgresql.conf and recovery.conf?

Why so? I made a clear proposal on how to proceed and this was the
first part of it.

You didn't answer the question. Does this get us any closer to merging
postgresql.conf and recovery.conf? Why is this bundled in with that?

Why do you think these points are bundled? It clearly isn't and I've
not claimed it gets us any closer to that goal.

But it is the first part of two agreed changes. And I am now working
on the second, which is the recovery.conf GUCs.

ISTM the quickest way forward is to revert this, and proceed with the rest
of the plan: get Michael/Fujii's patch into shape, and commit it. If we
still think this additional GUC is a good idea after that, we can add it
afterwards just as well.

My review of that patch is on file and my rejection of it clear for
all to see. I have proposed a way forwards, which achieved agreement
then and I have made it clear all the way that I would work on that,
and am now doing so. None of that is a surprise. And Fujii will
receive credit for his contribution, which is the bit where we make
recovery parms into GUCs.

The quickest way forward is to proceed with The Plan as agreed on Dec
24 - Jan 9. Restarting a discussion and formulating an alternative
plan is just deliberate interference with no justification, especially
not when we pretend that the later plan somehow supercedes the earlier
agreed one, and certainly not just because a few months went by in
between.

In summary, we have clear agreement that a file needs to trigger
recovery. We have no reason to believe that renaming the trigger file
to something else is a good thing, hence recovery.conf should remain
and its contents be treated as GUCs. And recovery.conf now has the
option of living in a different directory, which needs to be writable.
So we have the new features desired, plus backwards compatibility. And
off I go to code now.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#24Heikki Linnakangas
hlinnakangas@vmware.com
In reply to: Simon Riggs (#23)
Re: [COMMITTERS] pgsql: Allow external recovery_config_directory

On 27.03.2013 17:23, Simon Riggs wrote:

On 27 March 2013 14:20, Heikki Linnakangas<hlinnakangas@vmware.com> wrote:

You didn't answer the question. Does this get us any closer to merging
postgresql.conf and recovery.conf? Why is this bundled in with that?

Why do you think these points are bundled?

Because you say that this controversial commit is the 1st step before
the 2nd step, which is to actually merge postgresql.conf and recovery.conf.

It clearly isn't and I've
not claimed it gets us any closer to that goal.

Ok, cool. Can you please revert this commit so that we can move on, then?

But it is the first part of two agreed changes. And I am now working
on the second, which is the recovery.conf GUCs.

Thanks!

ISTM the quickest way forward is to revert this, and proceed with the rest
of the plan: get Michael/Fujii's patch into shape, and commit it. If we
still think this additional GUC is a good idea after that, we can add it
afterwards just as well.

My review of that patch is on file and my rejection of it clear for
all to see. I have proposed a way forwards, which achieved agreement
then and I have made it clear all the way that I would work on that,
and am now doing so. None of that is a surprise. And Fujii will
receive credit for his contribution, which is the bit where we make
recovery parms into GUCs.

Oh, ok. I thought the patch in the commitfest implemented what was
agreed on, but I admit I haven't looked at it closely.

In summary, we have clear agreement that a file needs to trigger
recovery. We have no reason to believe that renaming the trigger file
to something else is a good thing, hence recovery.conf should remain
and its contents be treated as GUCs.

I'm fine with that. But at least Robert just said he thinks the trigger
file should not be called recovery.conf, based on Greg Smith's earlier
proposal. I'm starting to feel that when we seemed to have a consensus
around Christmas, some people thought we agreed on one thing, and others
thought we agreed on something else.

For the record, I'm happy with calling the file recovery.conf, so that
it's backwards-compatible. I'm also happy with renaming it, per Greg
Smith's/Robert Haas' proposal.

And recovery.conf now has the option of living in a different
directory, which needs to be writable. So we have the new features
desired, plus backwards compatibility. And off I go to code now.

Yeah, we need to see the actual patch, so that everyone knows what
exactly is being proposed. In any case, it's independent of this commit.

- Heikki

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#25Simon Riggs
simon@2ndQuadrant.com
In reply to: Heikki Linnakangas (#24)
Re: [COMMITTERS] pgsql: Allow external recovery_config_directory

On 27 March 2013 15:35, Heikki Linnakangas <hlinnakangas@vmware.com> wrote:

On 27.03.2013 17:23, Simon Riggs wrote:

On 27 March 2013 14:20, Heikki Linnakangas<hlinnakangas@vmware.com>
wrote:

You didn't answer the question. Does this get us any closer to merging

postgresql.conf and recovery.conf? Why is this bundled in with that?

Why do you think these points are bundled?

Because you say that this controversial commit is the 1st step before the
2nd step, which is to actually merge postgresql.conf and recovery.conf.

At no point have I said this commit has anything to do with making
recovery.conf into GUCs, in fact, I said exactly that they were
separate things, though discussed on the same thread. Requesting a
revoke because they are *not* connected doesn't make sense.

It clearly isn't and I've
not claimed it gets us any closer to that goal.

Ok, cool. Can you please revert this commit so that we can move on, then?

Please explain why you want this reverted, without mentioning the
other task we agree is required.

This commit was an agreed upon, uncontroversial feature. What changed?

I'm fine with that. But at least Robert just said he thinks the trigger file
should not be called recovery.conf, based on Greg Smith's earlier proposal.
I'm starting to feel that when we seemed to have a consensus around
Christmas, some people thought we agreed on one thing, and others thought we
agreed on something else.

And recovery.conf now has the option of living in a different
directory, which needs to be writable. So we have the new features
desired, plus backwards compatibility. And off I go to code now.

Yeah, we need to see the actual patch, so that everyone knows what exactly
is being proposed. In any case, it's independent of this commit.

In the absence of reasons for change we leave things as they are.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#26Heikki Linnakangas
hlinnakangas@vmware.com
In reply to: Simon Riggs (#25)
Re: [COMMITTERS] pgsql: Allow external recovery_config_directory

On 27.03.2013 18:10, Simon Riggs wrote:

On 27 March 2013 15:35, Heikki Linnakangas<hlinnakangas@vmware.com> wrote:

Ok, cool. Can you please revert this commit so that we can move on, then?

Please explain why you want this reverted, without mentioning the
other task we agree is required.

If an admin can't trust that the file is placed in $PGDATA, it's harder
to determine if a server is a master or a standby. It makes tools that
try to promote / demote a server more complicated, because they need to
take this setting into account. Lastly, it breaks the new pg_basebackup
-R functionality; pg_basebackup will create the recovery.conf file, but
it won't take effect.

From a process standpoint, this is a new feature that should've been
submitted before the commitfest deadline. I'm sure we'll make exceptions
to that every now and then, but by default new features should be bumped
to the next release at this point.

- Heikki

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#27Tom Lane
tgl@sss.pgh.pa.us
In reply to: Heikki Linnakangas (#26)
Re: [COMMITTERS] pgsql: Allow external recovery_config_directory

Heikki Linnakangas <hlinnakangas@vmware.com> writes:

On 27.03.2013 18:10, Simon Riggs wrote:

On 27 March 2013 15:35, Heikki Linnakangas<hlinnakangas@vmware.com> wrote:

Ok, cool. Can you please revert this commit so that we can move on, then?

Please explain why you want this reverted, without mentioning the
other task we agree is required.

If an admin can't trust that the file is placed in $PGDATA, it's harder
to determine if a server is a master or a standby. It makes tools that
try to promote / demote a server more complicated, because they need to
take this setting into account. Lastly, it breaks the new pg_basebackup
-R functionality; pg_basebackup will create the recovery.conf file, but
it won't take effect.

FWIW, I agree that this is a bad idea and should be reverted.

Simon is claiming that because he described this idea in one sentence
(out of a larger post) three months ago, everyone agreed to the idea and
there is no longer any room for discussion. In reality I suspect nobody
really thought about the implications at the time. In any case, the
arguments that have been made today seem to me to be sufficient reasons
why we *don't* want to put recovery.conf in random places outside the
data directory.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#28Simon Riggs
simon@2ndQuadrant.com
In reply to: Heikki Linnakangas (#26)
Re: [COMMITTERS] pgsql: Allow external recovery_config_directory

On 27 March 2013 16:24, Heikki Linnakangas <hlinnakangas@vmware.com> wrote:

On 27.03.2013 18:10, Simon Riggs wrote:

On 27 March 2013 15:35, Heikki Linnakangas<hlinnakangas@vmware.com>
wrote:

Ok, cool. Can you please revert this commit so that we can move on, then?

Please explain why you want this reverted, without mentioning the
other task we agree is required.

If an admin can't trust that the file is placed in $PGDATA, it's harder to
determine if a server is a master or a standby. It makes tools that try to
promote / demote a server more complicated, because they need to take this
setting into account. Lastly, it breaks the new pg_basebackup -R
functionality; pg_basebackup will create the recovery.conf file, but it
won't take effect.

AFAIK pg_basebackup doesn't backup files not in the data directory and
tablespace dirs.

This doesn't change that. If it does, and I am mistaken, then it will
be an easy fix.

From a process standpoint, this is a new feature that should've been
submitted before the commitfest deadline. I'm sure we'll make exceptions to
that every now and then, but by default new features should be bumped to the
next release at this point.

I'm adding it by popular request, agreement and an explicit timing
plan, all of which I followed.

I didn't add this feature because I want it, and honestly, I don't
really care about it myself, which is why I left it to last thing on
my work schedule. But I do listen to requests from others, which is
why I've spent close to 2 days of my time on it as part of my
contribution to the team.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#29Heikki Linnakangas
hlinnakangas@vmware.com
In reply to: Simon Riggs (#28)
Re: [COMMITTERS] pgsql: Allow external recovery_config_directory

On 27.03.2013 19:34, Simon Riggs wrote:

On 27 March 2013 16:24, Heikki Linnakangas<hlinnakangas@vmware.com> wrote:

Lastly, it breaks the new pg_basebackup -R
functionality; pg_basebackup will create the recovery.conf file, but it
won't take effect.

AFAIK pg_basebackup doesn't backup files not in the data directory and
tablespace dirs.

Imagine that you have set recovery_config_directory='/foo/' in the
master server. Now you want to set up a standby, so you do:'

pg_basebackup -D data-standby -R

With the -R option, pg_basebackup creates data-standby/recovery.conf to
point to the master, with standby_mode='on'. But if you start the
server, it will start up as a master, not as a standby, because
recovery_config_directory points elsewhere.

- Heikki

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#30Simon Riggs
simon@2ndquadrant.com
In reply to: Tom Lane (#27)
Re: [COMMITTERS] pgsql: Allow external recovery_config_directory

On 27 March 2013 17:23, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Heikki Linnakangas <hlinnakangas@vmware.com> writes:

On 27.03.2013 18:10, Simon Riggs wrote:

On 27 March 2013 15:35, Heikki Linnakangas<hlinnakangas@vmware.com> wrote:

Ok, cool. Can you please revert this commit so that we can move on, then?

Please explain why you want this reverted, without mentioning the
other task we agree is required.

If an admin can't trust that the file is placed in $PGDATA, it's harder
to determine if a server is a master or a standby. It makes tools that
try to promote / demote a server more complicated, because they need to
take this setting into account. Lastly, it breaks the new pg_basebackup
-R functionality; pg_basebackup will create the recovery.conf file, but
it won't take effect.

FWIW, I agree that this is a bad idea and should be reverted.

Simon is claiming that because he described this idea in one sentence
(out of a larger post) three months ago, everyone agreed to the idea and
there is no longer any room for discussion. In reality I suspect nobody
really thought about the implications at the time. In any case, the
arguments that have been made today seem to me to be sufficient reasons
why we *don't* want to put recovery.conf in random places outside the
data directory.

If anybody thought one sentence wasn't descriptive enough, they could
have said. They didn't because its a trivial patch with very little
room for alternative interpretations.

Arguments against? I have seen only one, Heikki's above, and its not a
good one, given related similar issues.

It's an option, you don't have to put recovery.conf anywhere else,
unless you wish to.

Anyway, as I said, I didn't do this because I want it. I did it
because it's been agreed. Without some reasonable objection, I see no
reason to revoke.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#31Simon Riggs
simon@2ndQuadrant.com
In reply to: Heikki Linnakangas (#29)
Re: [COMMITTERS] pgsql: Allow external recovery_config_directory

On 27 March 2013 17:50, Heikki Linnakangas <hlinnakangas@vmware.com> wrote:

On 27.03.2013 19:34, Simon Riggs wrote:

On 27 March 2013 16:24, Heikki Linnakangas<hlinnakangas@vmware.com>
wrote:

Lastly, it breaks the new pg_basebackup -R

functionality; pg_basebackup will create the recovery.conf file, but it
won't take effect.

AFAIK pg_basebackup doesn't backup files not in the data directory and
tablespace dirs.

Imagine that you have set recovery_config_directory='/foo/' in the master
server. Now you want to set up a standby, so you do:'

pg_basebackup -D data-standby -R

With the -R option, pg_basebackup creates data-standby/recovery.conf to
point to the master, with standby_mode='on'. But if you start the server, it
will start up as a master, not as a standby, because
recovery_config_directory points elsewhere.

Yeh, I get it.

Same argument applies to all conf files, not just recovery.conf.

Sounds like the patch to add -R to pg_basebackup should be revoked as
being not well thought out. Or it should be fixed, in which case this
works the same.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#32Fujii Masao
masao.fujii@gmail.com
In reply to: Heikki Linnakangas (#26)
Re: [COMMITTERS] pgsql: Allow external recovery_config_directory

On Thu, Mar 28, 2013 at 1:24 AM, Heikki Linnakangas
<hlinnakangas@vmware.com> wrote:

On 27.03.2013 18:10, Simon Riggs wrote:

On 27 March 2013 15:35, Heikki Linnakangas<hlinnakangas@vmware.com>
wrote:

Ok, cool. Can you please revert this commit so that we can move on, then?

Please explain why you want this reverted, without mentioning the
other task we agree is required.

If an admin can't trust that the file is placed in $PGDATA, it's harder to
determine if a server is a master or a standby. It makes tools that try to
promote / demote a server more complicated, because they need to take this
setting into account. Lastly, it breaks the new pg_basebackup -R
functionality; pg_basebackup will create the recovery.conf file, but it
won't take effect.

This patch breaks also pg_ctl promote because it always checks the existence of
$PGDATA/recovery.conf.

Regards,

--
Fujii Masao

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#33Michael Paquier
michael.paquier@gmail.com
In reply to: Simon Riggs (#30)
Re: [COMMITTERS] pgsql: Allow external recovery_config_directory

On Thu, Mar 28, 2013 at 2:58 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

Arguments against?

Also the fact that many discussions have been done on recovery.conf between
the time this feature has been decided and actually committed (perhaps too
promptly just by looking at how this thread is becoming long)?
--
Michael

#34Simon Riggs
simon@2ndQuadrant.com
In reply to: Fujii Masao (#32)
Re: [COMMITTERS] pgsql: Allow external recovery_config_directory

On 27 March 2013 18:15, Fujii Masao <masao.fujii@gmail.com> wrote:

This patch breaks also pg_ctl promote because it always checks the existence of
$PGDATA/recovery.conf.

You're right. It does. Good catch.

That also suggest a solution: create a status file called
$PGDATA/recovery_in_progress which would then allow pg_ctl and
everybody else to be able to see that the server is currently in
recovery.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#35Heikki Linnakangas
hlinnakangas@vmware.com
In reply to: Simon Riggs (#31)
Re: [COMMITTERS] pgsql: Allow external recovery_config_directory

On 27.03.2013 20:02, Simon Riggs wrote:

On 27 March 2013 17:50, Heikki Linnakangas<hlinnakangas@vmware.com> wrote:

Imagine that you have set recovery_config_directory='/foo/' in the master
server. Now you want to set up a standby, so you do:'

pg_basebackup -D data-standby -R

With the -R option, pg_basebackup creates data-standby/recovery.conf to
point to the master, with standby_mode='on'. But if you start the server, it
will start up as a master, not as a standby, because
recovery_config_directory points elsewhere.

Yeh, I get it.

Same argument applies to all conf files, not just recovery.conf.

Sounds like the patch to add -R to pg_basebackup should be revoked as
being not well thought out. Or it should be fixed, in which case this
works the same.

What exactly was wrong with pg_basebackup -R, without
recovery_config_directory?

- Heikki

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#36Kevin Grittner
kgrittn@ymail.com
In reply to: Simon Riggs (#1)
Re: [COMMITTERS] pgsql: Allow external recovery_config_directory

Simon Riggs <simon@2ndQuadrant.com> wrote:

doc/src/sgml/config.sgml          |  17 +++++++++++++++++

So that doc builds could proceed without error I fixed the obvious
pasto in config.sgml.

--
Kevin Grittner
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#37Simon Riggs
simon@2ndQuadrant.com
In reply to: Heikki Linnakangas (#35)
Re: [COMMITTERS] pgsql: Allow external recovery_config_directory

On 27 March 2013 21:05, Heikki Linnakangas <hlinnakangas@vmware.com> wrote:

Same argument applies to all conf files, not just recovery.conf.

Sounds like the patch to add -R to pg_basebackup should be revoked as
being not well thought out. Or it should be fixed, in which case this
works the same.

What exactly was wrong with pg_basebackup -R, without
recovery_config_directory?

You asked me to answer the question. I did. Please answer mine.

Why do you think recovery_config_directory are different to
config_file with respect to pg_basebackup?
It looks like you've discovered a problem with pg_basebackup, not a
problem with this patch.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#38Michael Paquier
michael.paquier@gmail.com
In reply to: Heikki Linnakangas (#35)
Re: [COMMITTERS] pgsql: Allow external recovery_config_directory

On Thu, Mar 28, 2013 at 6:05 AM, Heikki Linnakangas <hlinnakangas@vmware.com

wrote:

What exactly was wrong with pg_basebackup -R, without
recovery_config_directory?

pg_basebackup -R generates automatically recovery.conf inside data folder,
so if
recovery_config_directory is specified the slave startup will fail.
The same problem exists also with the case where a tarball is generated for
base backup.
Such limitations should be specified in the docs at least.
--
Michael

#39Heikki Linnakangas
hlinnakangas@vmware.com
In reply to: Simon Riggs (#37)
Re: [COMMITTERS] pgsql: Allow external recovery_config_directory

On 27.03.2013 23:36, Simon Riggs wrote:

Why do you think recovery_config_directory are different to
config_file with respect to pg_basebackup?

pg_basebackup doesn't try to *write* anything to config_file. It does
write to $PGDATA/recovery.conf, with the expectation that it takes
effect when you start the server.

When you take a backup, I think it's quite reasonable that if you have
set config_file to a different location, that's not backed up. Same with
recovery.conf. But when pg_basebackup *creates* recovery.conf, it's at
least surprising if it doesn't take effect.

- Heikki

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#40Simon Riggs
simon@2ndQuadrant.com
In reply to: Heikki Linnakangas (#39)
Re: [COMMITTERS] pgsql: Allow external recovery_config_directory

On 28 March 2013 08:31, Heikki Linnakangas <hlinnakangas@vmware.com> wrote:

On 27.03.2013 23:36, Simon Riggs wrote:

Why do you think recovery_config_directory are different to
config_file with respect to pg_basebackup?

pg_basebackup doesn't try to *write* anything to config_file. It does write
to $PGDATA/recovery.conf, with the expectation that it takes effect when you
start the server.

When you take a backup, I think it's quite reasonable that if you have set
config_file to a different location, that's not backed up. Same with
recovery.conf. But when pg_basebackup *creates* recovery.conf, it's at least
surprising if it doesn't take effect.

No, it *would* take effect. The parameter is set in a config file that
is not part of the backup, so if you start the server from the backup
then it doesn't know that the recovery_config_directory had been set
and so it would read the recovery.conf written by pg_basebackup. If
you backup the parameter files as well, then it would fail, but that
is easily handled by saying the recovery.conf can exist either in
datadir or recovery_config_directory. I don't see that as a major
problem, or change. But for other reasons, I have revoked the patch.

You have highlighted that pg_basebackup does *not* take a fully
working backup in all cases, especially the -R case where it is
clearly broken. It is that pre-existing issue that leads to your
complaint, not the patch itself.

Please admit that by adding a line to the docs to explain the
non-working case of -R.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#41Robert Haas
robertmhaas@gmail.com
In reply to: Simon Riggs (#40)
Re: [COMMITTERS] pgsql: Allow external recovery_config_directory

On Thu, Mar 28, 2013 at 6:23 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

No, it *would* take effect. The parameter is set in a config file that
is not part of the backup, so if you start the server from the backup
then it doesn't know that the recovery_config_directory had been set
and so it would read the recovery.conf written by pg_basebackup.

Are you saying pg_basebackup doesn't back up postgresql.conf? I thought it did.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#42Simon Riggs
simon@2ndQuadrant.com
In reply to: Robert Haas (#41)
Re: [COMMITTERS] pgsql: Allow external recovery_config_directory

On 28 March 2013 11:36, Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Mar 28, 2013 at 6:23 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

No, it *would* take effect. The parameter is set in a config file that
is not part of the backup, so if you start the server from the backup
then it doesn't know that the recovery_config_directory had been set
and so it would read the recovery.conf written by pg_basebackup.

Are you saying pg_basebackup doesn't back up postgresql.conf? I thought it did.

postgresql.conf will be backed up if it is present in the data
directory. If it is not present, it is not backed up.

Therefore anybody using pg_basebackup and the config_file parameter
does *not* have an executable backup when used with the -R option, as
Heikki was suggesting was a requirement for this patch. So if we
regard that as a bug with the patch, then there is a bug with -R
with/without the patch.

pg_basebackup's behaviour with respect to .conf files is undocumented
so its a "feature" that it skips .conf files in the config_file case.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#43Heikki Linnakangas
hlinnakangas@vmware.com
In reply to: Simon Riggs (#42)
1 attachment(s)
Re: [COMMITTERS] pgsql: Allow external recovery_config_directory

On 28.03.2013 14:45, Simon Riggs wrote:

On 28 March 2013 11:36, Robert Haas<robertmhaas@gmail.com> wrote:

On Thu, Mar 28, 2013 at 6:23 AM, Simon Riggs<simon@2ndquadrant.com> wrote:

No, it *would* take effect. The parameter is set in a config file that
is not part of the backup, so if you start the server from the backup
then it doesn't know that the recovery_config_directory had been set
and so it would read the recovery.conf written by pg_basebackup.

Are you saying pg_basebackup doesn't back up postgresql.conf? I thought it did.

postgresql.conf will be backed up if it is present in the data
directory. If it is not present, it is not backed up.

Right.

Therefore anybody using pg_basebackup and the config_file parameter
does *not* have an executable backup when used with the -R option, as
Heikki was suggesting was a requirement for this patch.

That's not related to the -R option, the situation with config_file is
the same with or without it. But yes, if you use config_file option to
point outside the data directory, the config file won't be backed up.
That feels intuitive to me, I wouldn't expect it to be. Same with
include or include_dir directives in the config file, as well as
hba_file and ident_file - I wouldn't expect any of the files that those
point to to be included in the backup.

The filesystem-level backup procedure documented in the user manual, not
using pg_basebackup, behaves the same.

pg_basebackup's behaviour with respect to .conf files is undocumented
so its a "feature" that it skips .conf files in the config_file case.

Ok. It's always beem self-evident to me, but I agree it should be
documented. How about the attached?

- Heikki

Attachments:

document-backup-of-config-files-outside-datadir.patchtext/x-diff; name=document-backup-of-config-files-outside-datadir.patchDownload
diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml
index e444b1c..987802a 100644
--- a/doc/src/sgml/backup.sgml
+++ b/doc/src/sgml/backup.sgml
@@ -920,7 +920,10 @@ SELECT pg_stop_backup();
     If you are using tablespaces that do not reside underneath this directory,
     be careful to include them as well (and be sure that your backup dump
     archives symbolic links as links, otherwise the restore will corrupt
-    your tablespaces).
+    your tablespaces). If you have relocated any configuration files
+    outside the database cluster directory (see
+    <xref linkend="config-includes"> and
+    <xref linkend="runtime-config-file-locations">), include them as well.
    </para>
 
    <para>
diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index 9fe440a..bdf74a0 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -512,7 +512,11 @@ PostgreSQL documentation
    The backup will include all files in the data directory and tablespaces,
    including the configuration files and any additional files placed in the
    directory by third parties. Only regular files and directories are allowed
-   in the data directory, no symbolic links or special device files.
+   in the data directory, no symbolic links or special device files. If you
+   have relocated any configuration files outside the data directory (see
+   <xref linkend="config-includes"> and
+   <xref linkend="runtime-config-file-locations">), they will not be included
+   in the backup.
   </para>
 
   <para>
#44Simon Riggs
simon@2ndQuadrant.com
In reply to: Heikki Linnakangas (#43)
Re: [COMMITTERS] pgsql: Allow external recovery_config_directory

On 28 March 2013 13:47, Heikki Linnakangas <hlinnakangas@vmware.com> wrote:

Therefore anybody using pg_basebackup and the config_file parameter
does *not* have an executable backup when used with the -R option, as
Heikki was suggesting was a requirement for this patch.

That's not related to the -R option, the situation with config_file is the
same with or without it.

Yes, it is related to -R. You said that with -R we are expecting to
have a fully executable backup, which won't happen because the config
files are missing.

That is the situation now and the fact the patch had the same issue is
not relevant. It's a pre-existing issue.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#45Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Heikki Linnakangas (#43)
Re: [COMMITTERS] pgsql: Allow external recovery_config_directory

Heikki Linnakangas <hlinnakangas@vmware.com> writes:

That's not related to the -R option, the situation with config_file is the
same with or without it. But yes, if you use config_file option to point
outside the data directory, the config file won't be backed up. That feels
intuitive to me, I wouldn't expect it to be. Same with include or

It's a pain when using debian. I think pg_basebackup should copy the
configuration files in the destination directory by default, with an
option to tell it where to store them. Or at least it should issue some
client side warnings when the configuration files are known not to be
included in the backup.

The reason why copying to the destination directory is a good default is
that the debian tool pg_createcluster will then install those
configuration file in the "proper" place in /etc. So that the procedure
would become:

pg_basebackup -D dest ...
pg_createcluster 9.3 main dest
pg_ctlcluster 9.3 main start

And you now have a working standby. Whereas currently you need to add
some extra manual steps to cover the configuration.

include_dir directives in the config file, as well as hba_file and
ident_file - I wouldn't expect any of the files that those point to to be
included in the backup.

Why?

The filesystem-level backup procedure documented in the user manual, not
using pg_basebackup, behaves the same.

You can't expect filesystem-level procedures to know that kind of
details, or if you want those to, then use symlinks. On the other hand
the PostgreSQL tools should know to use the pg_settings view.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers