Thread: 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

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

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

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:

http://www.postgresql.org/message-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

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:
>
> http://www.postgresql.org/message-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

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

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:
>>
>> http://www.postgresql.org/message-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

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.

http://www.postgresql.org/message-id/20130109204225.GB21747@momjian.us
http://www.postgresql.org/message-id/20121224171529.GB19778@alap2.fritz.box

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

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.
>
> http://www.postgresql.org/message-id/20130109204225.GB21747@momjian.us
> http://www.postgresql.org/message-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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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