Re: [HACKERS] Turning recovery.conf into GUCs

2015-04-30 Thread Michael Paquier
On Sat, Feb 21, 2015 at 6:45 AM, Peter Eisentraut pete...@gmx.net wrote:
 On 2/19/15 4:33 PM, Josh Berkus wrote:
 On 02/19/2015 12:23 PM, Peter Eisentraut wrote:
 On 1/6/15 4:22 PM, Peter Eisentraut wrote:
 That said, there is a much simpler way to achieve that specific
 functionality: Expose all the recovery settings as fake read-only GUC
 variables.  See attached patch for an example.

 The commit fest app has this as the patch of record.  I don't think
 there is a real updated patch under consideration here.  This item
 should probably not be in the commit fest at all.

 What happened to the original patch?  Regardless of what we do, keeping
 recovery.conf the way it is can't be what we go forward with.

 There was disagreement on many of the details, and no subsequent new
 proposal.

Patch has been marked as Returned with feedback.
-- 
Michael


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


Re: [HACKERS] Turning recovery.conf into GUCs

2015-02-19 Thread Peter Eisentraut
On 1/6/15 4:22 PM, Peter Eisentraut wrote:
 That said, there is a much simpler way to achieve that specific
 functionality: Expose all the recovery settings as fake read-only GUC
 variables.  See attached patch for an example.

The commit fest app has this as the patch of record.  I don't think
there is a real updated patch under consideration here.  This item
should probably not be in the commit fest at all.



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


Re: [HACKERS] Turning recovery.conf into GUCs

2015-02-19 Thread Josh Berkus
On 02/19/2015 12:23 PM, Peter Eisentraut wrote:
 On 1/6/15 4:22 PM, Peter Eisentraut wrote:
 That said, there is a much simpler way to achieve that specific
 functionality: Expose all the recovery settings as fake read-only GUC
 variables.  See attached patch for an example.
 
 The commit fest app has this as the patch of record.  I don't think
 there is a real updated patch under consideration here.  This item
 should probably not be in the commit fest at all.

What happened to the original patch?  Regardless of what we do, keeping
recovery.conf the way it is can't be what we go forward with.

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


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


Re: [HACKERS] Turning recovery.conf into GUCs

2015-01-16 Thread Michael Paquier
On Wed, Jan 7, 2015 at 6:22 AM, Peter Eisentraut pete...@gmx.net wrote:
 I have written similar logic, and while it's not pleasant, it's doable.
  This issue would really only go away if you don't use a file to signal
 recovery at all, which you have argued for, but which is really a
 separate and more difficult problem.
Moving this patch to the next CF and marking it as returned with
feedback for current CF as there is visibly no consensus reached.
-- 
Michael


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


Re: [HACKERS] Turning recovery.conf into GUCs

2015-01-16 Thread Andres Freund
On 2015-01-16 21:43:43 +0900, Michael Paquier wrote:
 On Wed, Jan 7, 2015 at 6:22 AM, Peter Eisentraut pete...@gmx.net wrote:
  I have written similar logic, and while it's not pleasant, it's doable.
   This issue would really only go away if you don't use a file to signal
  recovery at all, which you have argued for, but which is really a
  separate and more difficult problem.
 Moving this patch to the next CF and marking it as returned with
 feedback for current CF as there is visibly no consensus reached.

I don't think that's a good idea. If we defer this another couple months
we'l *never* reach anything coming close to concensus.

Greetings,

Andres Freund

-- 
 Andres Freund 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


Re: [HACKERS] Turning recovery.conf into GUCs

2015-01-16 Thread Michael Paquier
On Fri, Jan 16, 2015 at 9:45 PM, Andres Freund and...@2ndquadrant.com wrote:
 On 2015-01-16 21:43:43 +0900, Michael Paquier wrote:
 On Wed, Jan 7, 2015 at 6:22 AM, Peter Eisentraut pete...@gmx.net wrote:
  I have written similar logic, and while it's not pleasant, it's doable.
   This issue would really only go away if you don't use a file to signal
  recovery at all, which you have argued for, but which is really a
  separate and more difficult problem.
 Moving this patch to the next CF and marking it as returned with
 feedback for current CF as there is visibly no consensus reached.

 I don't think that's a good idea. If we defer this another couple months
 we'l *never* reach anything coming close to concensus.
What makes you think that the situation could move suddendly move into
a direction more than another?
(FWIW, my vote goes to the all GUC approach with standby.enabled.)
-- 
Michael


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


Re: [HACKERS] Turning recovery.conf into GUCs

2015-01-16 Thread Andres Freund
On 2015-01-16 21:50:16 +0900, Michael Paquier wrote:
 On Fri, Jan 16, 2015 at 9:45 PM, Andres Freund and...@2ndquadrant.com wrote:
  On 2015-01-16 21:43:43 +0900, Michael Paquier wrote:
  On Wed, Jan 7, 2015 at 6:22 AM, Peter Eisentraut pete...@gmx.net wrote:
   I have written similar logic, and while it's not pleasant, it's doable.
This issue would really only go away if you don't use a file to signal
   recovery at all, which you have argued for, but which is really a
   separate and more difficult problem.
  Moving this patch to the next CF and marking it as returned with
  feedback for current CF as there is visibly no consensus reached.
 
  I don't think that's a good idea. If we defer this another couple months
  we'l *never* reach anything coming close to concensus.

 What makes you think that the situation could move suddendly move into
 a direction more than another?

That we have to fix this.

I see absolutely no advantage of declaring the discussion closed for
now. That doesn't exactly increase the chance of this ever succeeding.

Greetings,

Andres Freund

-- 
 Andres Freund 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


Re: [HACKERS] Turning recovery.conf into GUCs

2015-01-14 Thread Josh Berkus
On 01/15/2015 02:24 AM, Robert Haas wrote:
 I think your idea of adding read-only GUCs with the same names as all
 of the recovey.conf parameters is a clear win.  Even if we do nothing
 more than that, it makes the values visible from the SQL level, and
 that's good.  But I think we should go further and make them really be
 GUCs.  Otherwise, if you want to be able to change one of those
 parameters other than at server startup time, you've got to invent a
 separate system for reloading them on SIGHUP.  If you make them part
 of the GUC mechanism, you can use that.  I think it's quite safe to
 say that the whole reason we *have* a GUC mechanism is so that all of
 our configuration can be done through one grand, unified mechanism
 rather than being fragmented across many files.

+1

I do find it ironic that the creator of the Grand Unified Configuration
Settings is arguing against unifying the files.

 Some renaming might be in order.  Heikki previously suggested merging
 all of the recovery_target_whatever settings down into a single
 parameter recovery_target='kindofrecoverytarget furtherdetailsgohere',
 like recovery_target='xid 1234' or recovery_target='name bob'.  Maybe
 that would be more clear (or not). 

Not keen on non-atomic settings, personally.

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


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


Re: [HACKERS] Turning recovery.conf into GUCs

2015-01-14 Thread Robert Haas
On Mon, Jan 5, 2015 at 8:43 PM, Peter Eisentraut pete...@gmx.net wrote:
 I have previously argued for a different approach: Ready recovery.conf
 as a configuration file after postgresql.conf, but keep it as a file to
 start recovery.  That doesn't break any old workflows, it gets you all
 the advantages of having recovery parameters in the GUC system, and it
 keeps all the options open for improving things further in the future.

But this is confusing, too, because it means that if you set a
parameter in both postgresql.conf and recovery.conf, you'll end up
with the recovery.conf value of the parameter even after recovery is
done.  Until you restart, and then you won't.  That's weird.

I think your idea of adding read-only GUCs with the same names as all
of the recovey.conf parameters is a clear win.  Even if we do nothing
more than that, it makes the values visible from the SQL level, and
that's good.  But I think we should go further and make them really be
GUCs.  Otherwise, if you want to be able to change one of those
parameters other than at server startup time, you've got to invent a
separate system for reloading them on SIGHUP.  If you make them part
of the GUC mechanism, you can use that.  I think it's quite safe to
say that the whole reason we *have* a GUC mechanism is so that all of
our configuration can be done through one grand, unified mechanism
rather than being fragmented across many files.

Some renaming might be in order.  Heikki previously suggested merging
all of the recovery_target_whatever settings down into a single
parameter recovery_target='kindofrecoverytarget furtherdetailsgohere',
like recovery_target='xid 1234' or recovery_target='name bob'.  Maybe
that would be more clear (or not).  Maybe standby_mode needs a better
name.  But I think the starting point for this discussion shouldn't be
why in the world would we merge recovery.conf into postgresql.conf?
but why, when we've otherwise gone to such trouble to push all of our
configuration through a single, unified mechanism that offers many
convenient features, do we continue to suffer our recovery.conf
settings to go through some other, less-capable mechanism?.

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


Re: [HACKERS] Turning recovery.conf into GUCs

2015-01-14 Thread Petr Jelinek

On 14/01/15 14:24, Robert Haas wrote:

On Mon, Jan 5, 2015 at 8:43 PM, Peter Eisentraut pete...@gmx.net wrote:

I have previously argued for a different approach: Ready recovery.conf
as a configuration file after postgresql.conf, but keep it as a file to
start recovery.  That doesn't break any old workflows, it gets you all
the advantages of having recovery parameters in the GUC system, and it
keeps all the options open for improving things further in the future.


But this is confusing, too, because it means that if you set a
parameter in both postgresql.conf and recovery.conf, you'll end up
with the recovery.conf value of the parameter even after recovery is
done.  Until you restart, and then you won't.  That's weird.

I think your idea of adding read-only GUCs with the same names as all
of the recovey.conf parameters is a clear win.  Even if we do nothing
more than that, it makes the values visible from the SQL level, and
that's good.  But I think we should go further and make them really be
GUCs.  Otherwise, if you want to be able to change one of those
parameters other than at server startup time, you've got to invent a
separate system for reloading them on SIGHUP.  If you make them part
of the GUC mechanism, you can use that.  I think it's quite safe to
say that the whole reason we *have* a GUC mechanism is so that all of
our configuration can be done through one grand, unified mechanism
rather than being fragmented across many files.


This is basically what the patch which is in commitfest does no?



Some renaming might be in order.  Heikki previously suggested merging
all of the recovery_target_whatever settings down into a single
parameter recovery_target='kindofrecoverytarget furtherdetailsgohere',
like recovery_target='xid 1234' or recovery_target='name bob'.  Maybe
that would be more clear (or not).


I was thinking about this a lot myself while reviewing the patch too, 
seems strange to have multiple config parameters for what is essentially 
same thing, my thinking was similar except I'd use : as separator 
('kindofrecoverytarget:furtherdetailsgohere'). I think though while it 
is technically nicer to do this, it might hurt usability for users.



Maybe standby_mode needs a better name.


I actually think standby_mode should be merged with hot_standby (as in 
standby_mode = 'hot'/'warm'/'off' or something).



But I think the starting point for this discussion shouldn't be
why in the world would we merge recovery.conf into postgresql.conf?
but why, when we've otherwise gone to such trouble to push all of our
configuration through a single, unified mechanism that offers many
convenient features, do we continue to suffer our recovery.conf
settings to go through some other, less-capable mechanism?.



+1

--
 Petr Jelinek  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


Re: [HACKERS] Turning recovery.conf into GUCs

2015-01-08 Thread Josh Berkus
On 01/08/2015 12:57 PM, Peter Eisentraut wrote:
  c) Infrastructure for changing settings effective during recovery. Right
 now we'd have to rebuild a lot of guc infrasturcture to allow
 that. It'd not be that hard to allow changing parameters like
 restore_command, primary_conninfo, recovery_target_* et al. That's
 for sure not the same commit, but once the infrastructure is in those
 won't be too hard.
 Right, if that worked, then it would be a real win.  But this discussion
 is about right now, and the perspective of the user.

That's rather a catch-22, isn't it?

Last I checked, it was our policy to try to get smaller, more discrete
patches rather than patches which try to change everything at once.

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


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


Re: [HACKERS] Turning recovery.conf into GUCs

2015-01-08 Thread Peter Eisentraut
On 1/6/15 4:40 PM, Josh Berkus wrote:
 Btw., I'm not sure that everyone will be happy to have primary_conninfo
  visible, since it might contain passwords.
 Didn't we discuss this?  I forgot what the conclusion was ... probably
 not to put passwords in primary_conninfo.

One can always say, don't do that then.  But especially with
pg_basebackup -R mindlessly copying passwords from .pgpass into
recovery.conf, the combination of these factors would proliferate
passwords a bit too easily for my taste.

Maybe a separate primary_conninfo_password that is a kind of write-only
GUC would work.  (That's how passwords usually work: You can change your
password, but can't see your existing one.)



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


Re: [HACKERS] Turning recovery.conf into GUCs

2015-01-08 Thread Peter Eisentraut
On 1/6/15 8:08 PM, Andres Freund wrote:
 On 2015-01-05 20:43:12 -0500, Peter Eisentraut wrote:
 For example, putting recovery target parameters into postgresql.conf
 might not make sense to some people.  Once you have reached the recovery
 end point, the information is obsolete.  Keeping it set could be
 considered confusing.
 
 I don't know, but I think that ship has sailed. hot_standby,
 archive_command, archive_mode, hot_standby_feedback are all essentially
 treated differently between primary and standby.

I don't mind those.  I mean things like recovery_target_time.

 Moreover, when I'm actually doing point-in-time-recovery attempts, I
 don't think I want to be messing with my pristine postgresql.conf file.
  Having those settings separate isn't a bad idea in that case.
 
 Well, nothing stops you from having a include file or something similar.

Sure, I need to update postgresql.conf to have an include file.

 I think we should just make recovery.conf behave exactly the way it does
 right now, except parse it according to guc rules. That way the changes
 when migrating are minimal and we don't desupport any
 usecases. Disallowing that way of operating just seems like
 intentionally throwing rocks in the way of getting this done.

That was more or less my proposal.

 The current system makes it easy to share postgresql.conf between
 primary and standby and just maintain the information related to the
 standby locally in recovery.conf.  pg_basebackup -R makes that even
 easier.  It's still possible to do this in the new system, but it's
 decidedly more work.
 
 Really? Howso?

You have to set up include files, override the include file on the
standby, I don't know how pg_basebackup -R would even work.  And most
importantly, you have to come up with all of that yourself, instead of
it just working.

 The wins on the other hand are obscure: You can now use SHOW to inspect
 recovery settings.  You can design your own configuration file include
 structures to set them.  These are not bad, but is that all?
 
 It's much more:
 a) One configuration format instead of two somewhat, but not really,
similar ones.

Agreed, but that's also fixable by just changing how recovery.conf is
parsed.  It doesn't require user space changes.

 b) Proper infrastructure to deal with configuration variable boundaries
and such. Just a few days ago there was e7c11887 et al.

That's just an implementation issue.

 c) Infrastructure for changing settings effective during recovery. Right
now we'd have to rebuild a lot of guc infrasturcture to allow
that. It'd not be that hard to allow changing parameters like
restore_command, primary_conninfo, recovery_target_* et al. That's
for sure not the same commit, but once the infrastructure is in those
won't be too hard.

Right, if that worked, then it would be a real win.  But this discussion
is about right now, and the perspective of the user.



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


Re: [HACKERS] Turning recovery.conf into GUCs

2015-01-07 Thread Josh Berkus
On 01/07/2015 02:31 PM, Peter Eisentraut wrote:
 On 1/6/15 7:33 PM, Josh Berkus wrote:
 D. Wierd name: for those doing only replication, not PITR,
 recovery.conf is completely baffling.
 
 I don't disagree, but standby.enabled or whatever isn't any better,
 for the inverse reason.

Yeah.  Like I said, I posted a list of bugs and features so that we can
test your solution and the pg.conf merger to see how much they actually
improve things.

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


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


Re: [HACKERS] Turning recovery.conf into GUCs

2015-01-07 Thread Peter Eisentraut
On 1/6/15 7:33 PM, Josh Berkus wrote:
 D. Wierd name: for those doing only replication, not PITR,
 recovery.conf is completely baffling.

I don't disagree, but standby.enabled or whatever isn't any better,
for the inverse reason.

But replication and PITR are the same thing, so any name is going to
have that problem.

One way out of that would be to develop higher-level abstractions, like
pg_ctl start -m standby or something, similar to how pg_ctl promote is
an abstraction and got people away from fiddling with trigger files.



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


Re: [HACKERS] Turning recovery.conf into GUCs

2015-01-07 Thread Kevin Grittner
Andres Freund and...@2ndquadrant.com wrote:
 On 2015-01-06 17:08:20 -0800, Josh Berkus wrote:
 On 01/06/2015 04:42 PM, Andres Freund wrote:
 On 2015-01-06 16:33:36 -0800, Josh Berkus wrote:
 F. Inability to remaster without restarting the replica.

 That has pretty much nothing to do with the topic at hand.

 It has *everything* to do with the topic at hand.  The *only* reason we
 can't remaster without restarting is that recovery.conf is only read at
 startup time, and can't be reloaded.

 http://www.databasesoup.com/2014/05/remastering-without-restarting.html

 Not really. It's a good way to introduce suble and hard to understand
 corruption and other strange corner cases. Your replication connection
 was lost/reconnected in the wrong moment? Oops, received/replayed too
 much.

 A real solution to this requires more. You need to 1) disable writing
 any wal 2) force catchup of all possible standbys, including apply. Most
 importantly of the new master. This requires knowing which standbys
 exist. 3) promote new master. 4) only now allow reconnects.

I'm not following.  As long as each standby knows what point it is
at in the transaction stream, it could make a request to any
transaction source for transactions past that point.  If a node
which will be promoted to the new master isn't caught up to that
point, it should not send anything.  When it does get caught up it
could start sending transactions past that point, including the
timeline switch when it is promoted.

If you were arguing that some changes besides *just* allowing a
standby to read from a new source without a restart, OK -- some
changes might also be needed for a promoted master to behave as
described above; but certainly the ability to read WAL from a new
source on the floor would be a prerequisite, and possibly the
biggest hurdle to clear.

--
Kevin Grittner
EDB: 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


Re: [HACKERS] Turning recovery.conf into GUCs

2015-01-06 Thread Josh Berkus
On 01/06/2015 01:22 PM, Peter Eisentraut wrote:

 That said, there is a much simpler way to achieve that specific
 functionality: Expose all the recovery settings as fake read-only GUC
 variables.  See attached patch for an example.

I have no objections to that idea in principle.  As long as it gets into
9.5.

 Btw., I'm not sure that everyone will be happy to have primary_conninfo
 visible, since it might contain passwords.

Didn't we discuss this?  I forgot what the conclusion was ... probably
not to put passwords in primary_conninfo.

 
 ... and there you hit on one of the other issues with recovery.conf,
 which is that it's a configuration file with configuration parameters
 which gets automatically renamed when a standby is promoted.  This plays
 merry hell with configuration management systems.  The amount of
 conditional logic I've had to write for Salt to handle recovery.conf
 truly doesn't bear thinking about.  There may be some other way to make
 recovery.conf configuration-management friendly, but I haven't thought
 of it.
 
 I have written similar logic, and while it's not pleasant, it's doable.
  This issue would really only go away if you don't use a file to signal
 recovery at all, which you have argued for, but which is really a
 separate and more difficult problem.

As far as CMSes are concerned, there is a vast difference between a file
which contains configuration variables and one which does not.  That is,
an *empty* recovery.conf file which just signals the start of recovery
is not a configuration problem.  The problem comes in in that
recovery.conf serves two separate purposes: it's a configuration file,
and it's also a trigger file.

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


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


Re: [HACKERS] Turning recovery.conf into GUCs

2015-01-06 Thread Peter Eisentraut
On 1/6/15 12:57 AM, Josh Berkus wrote:
 On 01/05/2015 05:43 PM, Peter Eisentraut wrote:
 The wins on the other hand are obscure: You can now use SHOW to inspect
 recovery settings.  You can design your own configuration file include
 structures to set them.  These are not bad, but is that all?
 
 That's not the only potential win, and it's not small either. I'll be
 able to tell what master a replica is replicating from using via a port
 5432 connection (currently there is absolutely no way to do this).

That's one particular case of what I mentioned above under using SHOW to
inspect recovery settings.  I agree that that's important, but it
doesn't look like there is a consensus that it justifies all the drawbacks.

That said, there is a much simpler way to achieve that specific
functionality: Expose all the recovery settings as fake read-only GUC
variables.  See attached patch for an example.

Btw., I'm not sure that everyone will be happy to have primary_conninfo
visible, since it might contain passwords.

 ... and there you hit on one of the other issues with recovery.conf,
 which is that it's a configuration file with configuration parameters
 which gets automatically renamed when a standby is promoted.  This plays
 merry hell with configuration management systems.  The amount of
 conditional logic I've had to write for Salt to handle recovery.conf
 truly doesn't bear thinking about.  There may be some other way to make
 recovery.conf configuration-management friendly, but I haven't thought
 of it.

I have written similar logic, and while it's not pleasant, it's doable.
 This issue would really only go away if you don't use a file to signal
recovery at all, which you have argued for, but which is really a
separate and more difficult problem.

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 218de87..477b906 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -237,10 +237,10 @@ static int	recovery_min_apply_delay = 0;
 static TimestampTz recoveryDelayUntilTime;
 
 /* options taken from recovery.conf for XLOG streaming */
-static bool StandbyModeRequested = false;
-static char *PrimaryConnInfo = NULL;
-static char *PrimarySlotName = NULL;
-static char *TriggerFile = NULL;
+bool StandbyModeRequested = false;
+char *PrimaryConnInfo = NULL;
+char *PrimarySlotName = NULL;
+char *TriggerFile = NULL;
 
 /* are we currently in standby mode? */
 bool		StandbyMode = false;
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index f6df077..6caadff 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -864,6 +864,15 @@ static struct config_bool ConfigureNamesBool[] =
 		NULL, NULL, NULL
 	},
 	{
+		{standby_mode, PGC_INTERNAL, WAL_SETTINGS,
+			gettext_noop(TODO),
+			NULL
+		},
+		StandbyModeRequested,
+		false,
+		NULL, NULL, NULL
+	},
+	{
 		{fsync, PGC_SIGHUP, WAL_SETTINGS,
 			gettext_noop(Forces synchronization of updates to disk.),
 			gettext_noop(The server will use the fsync() system call in several places to make 
@@ -3275,6 +3284,37 @@ static struct config_string ConfigureNamesString[] =
 	},
 
 	{
+		{primary_conninfo, PGC_INTERNAL, WAL_SETTINGS,
+			gettext_noop(TODO),
+			NULL,
+			GUC_SUPERUSER_ONLY
+		},
+		PrimaryConnInfo,
+		NULL,
+		NULL, NULL, NULL
+	},
+
+	{
+		{primary_slot_name, PGC_INTERNAL, WAL_SETTINGS,
+			gettext_noop(TODO),
+			NULL
+		},
+		PrimarySlotName,
+		NULL,
+		NULL, NULL, NULL
+	},
+
+	{
+		{trigger_file, PGC_INTERNAL, WAL_SETTINGS,
+			gettext_noop(TODO),
+			NULL
+		},
+		TriggerFile,
+		NULL,
+		NULL, NULL, NULL
+	},
+
+	{
 		{application_name, PGC_USERSET, LOGGING_WHAT,
 			gettext_noop(Sets the application name to be reported in statistics and logs.),
 			NULL,
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index 138deaf..eeb9461 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -100,6 +100,11 @@ extern bool fullPageWrites;
 extern bool wal_log_hints;
 extern bool log_checkpoints;
 
+extern bool StandbyModeRequested;
+extern char *PrimaryConnInfo;
+extern char *PrimarySlotName;
+extern char *TriggerFile;
+
 /* WAL levels */
 typedef enum WalLevel
 {

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


Re: [HACKERS] Turning recovery.conf into GUCs

2015-01-06 Thread Andres Freund
On 2015-01-06 16:33:36 -0800, Josh Berkus wrote:
 F. Inability to remaster without restarting the replica.

That has pretty much nothing to do with the topic at hand.

 I. Fairly duplicative options between pg.conf and recovery.conf (i.e.
 standby_mode vs. hot_standby)

Those aren't the same.

Greetings,

Andres Freund

-- 
 Andres Freund 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


Re: [HACKERS] Turning recovery.conf into GUCs

2015-01-06 Thread Josh Berkus
Peter, all:

Let me go over the issues I find with recovery.conf, based on 3 aspects
of my experience (1) doing DBA support (2) as a tool author (HandyRep)
and (3) as a trainer, teaching new users about it.  If we agree on a
list of problems with the current setup (as well as a list of benefits)
that's a good litmus test on whether any new version is worth adopting.
 Basically, new ways of doing this should remove some of the issues
while not jettisoning the benefits as much as possible.

Issues:

A. different formatting compared with PostgreSQL.conf, particularly
quoting, and lack of support for include files.

B. Inability to find out recovery.conf variables over port 5432.

C. Difficulty of management due to being both a trigger file and a
configuration file.

D. Wierd name: for those doing only replication, not PITR,
recovery.conf is completely baffling.

E. Replication/PITR confusion: some configuration items (particularly
recovery_target_*) are contradictory.

F. Inability to remaster without restarting the replica.

G. Inability to change parameters using ALTER SYSTEM SET.

H. Requirement of being in the data directory instead of the
configuration directory.

I. Fairly duplicative options between pg.conf and recovery.conf (i.e.
standby_mode vs. hot_standby)

Benefits:

1. Ability to share the exact same postgresql.conf for replica and master.

2. Easy pg_basebackup because you can exclude/generate a recovery.conf
automatically.

3. Battle-tested way to make sure that replication/recovery state
persists across unexpected restarts, and simple promotion workflow.


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


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


Re: [HACKERS] Turning recovery.conf into GUCs

2015-01-06 Thread Andres Freund
On 2015-01-05 20:43:12 -0500, Peter Eisentraut wrote:
 For example, putting recovery target parameters into postgresql.conf
 might not make sense to some people.  Once you have reached the recovery
 end point, the information is obsolete.  Keeping it set could be
 considered confusing.

I don't know, but I think that ship has sailed. hot_standby,
archive_command, archive_mode, hot_standby_feedback are all essentially
treated differently between primary and standby.

 Moreover, when I'm actually doing point-in-time-recovery attempts, I
 don't think I want to be messing with my pristine postgresql.conf file.
  Having those settings separate isn't a bad idea in that case.

Well, nothing stops you from having a include file or something similar.

I think we should just make recovery.conf behave exactly the way it does
right now, except parse it according to guc rules. That way the changes
when migrating are minimal and we don't desupport any
usecases. Disallowing that way of operating just seems like
intentionally throwing rocks in the way of getting this done.

 In the past, when some people have complained that recovery.conf cannot
 be moved to a configuration directory, I (and others?) have argued that
 recovery.conf is really more of a command script file than a
 configuration file (that was before replication was commonplace).  The
 premise of this patch is that some options really are more configuration
 than command most of the time, but that doesn't mean the old view was
 invalid.

Again, I think this ship has long since sailed.

 The current system makes it easy to share postgresql.conf between
 primary and standby and just maintain the information related to the
 standby locally in recovery.conf.  pg_basebackup -R makes that even
 easier.  It's still possible to do this in the new system, but it's
 decidedly more work.

Really? Howso?

 The wins on the other hand are obscure: You can now use SHOW to inspect
 recovery settings.  You can design your own configuration file include
 structures to set them.  These are not bad, but is that all?

It's much more:
a) One configuration format instead of two somewhat, but not really,
   similar ones.
b) Proper infrastructure to deal with configuration variable boundaries
   and such. Just a few days ago there was e7c11887 et al.
c) Infrastructure for changing settings effective during recovery. Right
   now we'd have to rebuild a lot of guc infrasturcture to allow
   that. It'd not be that hard to allow changing parameters like
   restore_command, primary_conninfo, recovery_target_* et al. That's
   for sure not the same commit, but once the infrastructure is in those
   won't be too hard.

 I note that all the new settings are PGC_POSTMASTER, so you can't set
 them on the fly.  Changing primary_conninfo without a restart would be a
 big win, for example.  Is that planned?

I think it's not too hard to do - but I'll fight hard to do that
separately. Once we've the infrastructure I'd be surprised if took too
long to change some of them to PGC_SIGHUP.

 I have previously argued for a different approach: Ready recovery.conf
 as a configuration file after postgresql.conf, but keep it as a file to
 start recovery.  That doesn't break any old workflows, it gets you all
 the advantages of having recovery parameters in the GUC system, and it
 keeps all the options open for improving things further in the future.

Well, it breaks because quoting and such is different. Otherwise I think
I agree. It allows you to keep parameters in recovery.conf if you want,
if not not.

If we add a recovery_file guc that defaults to '$PGDATA/recovery.conf'
we'd have a rather easy way to move forward imo. We can even allow two
filenames so we could default to something like
recovery_file = 'PGDATA/recovery.conf; PGDATA/recovery.trigger'

Greetings,

Andres Freund

-- 
 Andres Freund 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


Re: [HACKERS] Turning recovery.conf into GUCs

2015-01-06 Thread Josh Berkus
On 01/06/2015 05:17 PM, Andres Freund wrote:
 A real solution to this requires more. You need to 1) disable writing
 any wal 2) force catchup of all possible standbys, including apply. Most
 importantly of the new master. This requires knowing which standbys
 exist. 3) promote new master. 4) only now allow reconnect

That can all be handled externally to PostgreSQL.  However, as long as
we have a recovery.conf file which only gets read at server start, and
at no other time, no external solution is possible.

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


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


Re: [HACKERS] Turning recovery.conf into GUCs

2015-01-06 Thread Josh Berkus
On 01/06/2015 04:42 PM, Andres Freund wrote:
 On 2015-01-06 16:33:36 -0800, Josh Berkus wrote:
 F. Inability to remaster without restarting the replica.
 
 That has pretty much nothing to do with the topic at hand.

It has *everything* to do with the topic at hand.  The *only* reason we
can't remaster without restarting is that recovery.conf is only read at
startup time, and can't be reloaded.

http://www.databasesoup.com/2014/05/remastering-without-restarting.html

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


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


Re: [HACKERS] Turning recovery.conf into GUCs

2015-01-06 Thread Andres Freund
On 2015-01-06 17:08:20 -0800, Josh Berkus wrote:
 On 01/06/2015 04:42 PM, Andres Freund wrote:
  On 2015-01-06 16:33:36 -0800, Josh Berkus wrote:
  F. Inability to remaster without restarting the replica.
  
  That has pretty much nothing to do with the topic at hand.
 
 It has *everything* to do with the topic at hand.  The *only* reason we
 can't remaster without restarting is that recovery.conf is only read at
 startup time, and can't be reloaded.

 http://www.databasesoup.com/2014/05/remastering-without-restarting.html

Not really. It's a good way to introduce suble and hard to understand
corruption and other strange corner cases. Your replication connection
was lost/reconnected in the wrong moment? Oops, received/replayed too
much.

To achieve what you describe there, you don't even need a proxy, simple
dns based failover suffices.

A real solution to this requires more. You need to 1) disable writing
any wal 2) force catchup of all possible standbys, including apply. Most
importantly of the new master. This requires knowing which standbys
exist. 3) promote new master. 4) only now allow reconnects.

Greetings,

Andres Freund

-- 
 Andres Freund 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


Re: [HACKERS] Turning recovery.conf into GUCs

2015-01-05 Thread Peter Eisentraut
I think this is a valuable effort, but I wonder how we are going to
arrive at a consensus.  I don't see any committer supporting these
changes. Clearly, there are some advantages to having recovery
parameters be GUCs, but the proposed changes also come with some
disadvantages, and the tradeoffs aren't so clear.

For example, putting recovery target parameters into postgresql.conf
might not make sense to some people.  Once you have reached the recovery
end point, the information is obsolete.  Keeping it set could be
considered confusing.

Moreover, when I'm actually doing point-in-time-recovery attempts, I
don't think I want to be messing with my pristine postgresql.conf file.
 Having those settings separate isn't a bad idea in that case.

Some people might like the recovery.done file as a historical record.
Having standby.enabled just be deleted would destroy that information
once recovery has ended.

In the past, when some people have complained that recovery.conf cannot
be moved to a configuration directory, I (and others?) have argued that
recovery.conf is really more of a command script file than a
configuration file (that was before replication was commonplace).  The
premise of this patch is that some options really are more configuration
than command most of the time, but that doesn't mean the old view was
invalid.

The current system makes it easy to share postgresql.conf between
primary and standby and just maintain the information related to the
standby locally in recovery.conf.  pg_basebackup -R makes that even
easier.  It's still possible to do this in the new system, but it's
decidedly more work.

These are all soft reasons, but they add up.

The wins on the other hand are obscure: You can now use SHOW to inspect
recovery settings.  You can design your own configuration file include
structures to set them.  These are not bad, but is that all?

I note that all the new settings are PGC_POSTMASTER, so you can't set
them on the fly.  Changing primary_conninfo without a restart would be a
big win, for example.  Is that planned?

It might be acceptable to break all the old workflows if the new system
was obviously great, but it's not.  It's still confusing as heck.  For
example, we would have a file standby.enabled and a setting
standby_mode, which would mean totally different things.  I think
there is also some conceptual overlap between standby_mode and
recovery_target_action (standby_mode is really something like
recovery_target_action = keep_going).  I also find the various uses of
trigger file or to trigger confusing.  The old trigger file to
trigger promotion makes sense.  But calling standby.enabled a trigger
file as well confuses two entirely different concepts.

I have previously argued for a different approach: Ready recovery.conf
as a configuration file after postgresql.conf, but keep it as a file to
start recovery.  That doesn't break any old workflows, it gets you all
the advantages of having recovery parameters in the GUC system, and it
keeps all the options open for improving things further in the future.



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


Re: [HACKERS] Turning recovery.conf into GUCs

2015-01-05 Thread Josh Berkus
On 01/05/2015 05:43 PM, Peter Eisentraut wrote:
 The wins on the other hand are obscure: You can now use SHOW to inspect
 recovery settings.  You can design your own configuration file include
 structures to set them.  These are not bad, but is that all?

That's not the only potential win, and it's not small either. I'll be
able to tell what master a replica is replicating from using via a port
5432 connection (currently there is absolutely no way to do this).

 I note that all the new settings are PGC_POSTMASTER, so you can't set
 them on the fly.  Changing primary_conninfo without a restart would be a
 big win, for example.  Is that planned?

... but there you have the flaw in the ointment.  Unless we make some of
the settings superuserset, no, it doesn't win us a lot, except ...

 I have previously argued for a different approach: Ready recovery.conf
 as a configuration file after postgresql.conf, but keep it as a file to
 start recovery.  That doesn't break any old workflows, it gets you all
 the advantages of having recovery parameters in the GUC system, and it
 keeps all the options open for improving things further in the future.

... and there you hit on one of the other issues with recovery.conf,
which is that it's a configuration file with configuration parameters
which gets automatically renamed when a standby is promoted.  This plays
merry hell with configuration management systems.  The amount of
conditional logic I've had to write for Salt to handle recovery.conf
truly doesn't bear thinking about.  There may be some other way to make
recovery.conf configuration-management friendly, but I haven't thought
of it.

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


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


Re: [HACKERS] Turning recovery.conf into GUCs

2015-01-05 Thread Petr Jelinek

On 24/12/14 20:11, Alex Shulgin wrote:

Alex Shulgin a...@commandprompt.com writes:


- the StandbyModeRequested and StandbyMode should be lowercased like
the rest of the GUCs


Yes, except that standby_mode is linked to StandbyModeRequested, not the
other one.  I can try see if there's a sane way out of this.


As I see it, renaming these will only add noise to this patch, and there
is also ArchiveRecoveryRequested / InArchiveRecovery.  This is going to
be tricky and I'm not sure it's really worth the effort.



I don't buy that to be honest, most (if not all) places that would be 
affected are already in diff as part of context around other renames 
anyway and it just does not seem right to rename some variables and not 
the others.


I still think there should be some thought given to merging the 
hot_standby and standby_mode, but I think we'd need opinion of more 
people (especially some committers) on this one.



- The whole CopyErrorData and memory context logic is not needed in
check_recovery_target_time() as the FlushErrorState() is not called
there


You must be right.  I recall having some trouble with strings being
free'd prematurely, but if it's ERROR, then there should be no need for
that.  I'll check again.


Hrm, after going through this again I'm pretty sure that was correct:
the only way to obtain the current error message is to use
CopyErrorData(), but that requires you to switch back to non-error
memory context (via Assert).


Right, my bad.



The FlushErrorState() call is not there because it's handled by the hook
caller (or it can exit via ereport() with elevel==ERROR).



Yes I've seen that, shouldn't you FreeErrorData(edata) then though? I 
guess it does not matter too much considering that you are going to 
throw error and die eventually anyway once you are in that code path, 
maybe just for the clarity...


--
 Petr Jelinek  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


Re: [HACKERS] Turning recovery.conf into GUCs

2014-12-24 Thread Alex Shulgin
Alex Shulgin a...@commandprompt.com writes:

 Petr Jelinek p...@2ndquadrant.com writes:

 I had a quick look, the patch does not apply cleanly anymore but it's
 just release notes so nothing too bad.

 Yes, there were some ongoing changes that touched some parts of this and
 I must have missed the latest one (or maybe I was waiting for it to
 settle down).

The rebased version is attached.

 - the StandbyModeRequested and StandbyMode should be lowercased like
 the rest of the GUCs

 Yes, except that standby_mode is linked to StandbyModeRequested, not the
 other one.  I can try see if there's a sane way out of this.

As I see it, renaming these will only add noise to this patch, and there
is also ArchiveRecoveryRequested / InArchiveRecovery.  This is going to
be tricky and I'm not sure it's really worth the effort.

 - The whole CopyErrorData and memory context logic is not needed in
 check_recovery_target_time() as the FlushErrorState() is not called
 there

 You must be right.  I recall having some trouble with strings being
 free'd prematurely, but if it's ERROR, then there should be no need for
 that.  I'll check again.

Hrm, after going through this again I'm pretty sure that was correct:
the only way to obtain the current error message is to use
CopyErrorData(), but that requires you to switch back to non-error
memory context (via Assert).

The FlushErrorState() call is not there because it's handled by the hook
caller (or it can exit via ereport() with elevel==ERROR).

 - The new code in StartupXLOG() like
 +if (recovery_target_xid_string != NULL)
 +InitRecoveryTarget(RECOVERY_TARGET_XID);
 +
 +if (recovery_target_time_string != NULL)
 +InitRecoveryTarget(RECOVERY_TARGET_TIME);
 +
 +if (recovery_target_name != NULL)
 +InitRecoveryTarget(RECOVERY_TARGET_NAME);
 +
 +if (recovery_target_string != NULL)
 +InitRecoveryTarget(RECOVERY_TARGET_IMMEDIATE);

 would probably be better in separate function that you then call
 StartupXLOG() as StartupXLOG() is crazy long already so I think it's
 preferable to not make it worse.

 We can move it at top of CheckStartingAsStandby() obviously.

Moved.

--
Alex



recovery_guc_v5.6.patch.gz
Description: application/gzip

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


Re: [HACKERS] Turning recovery.conf into GUCs

2014-12-23 Thread Petr Jelinek

On 12/12/14 16:34, Alex Shulgin wrote:

Alex Shulgin a...@commandprompt.com writes:


Alex Shulgin a...@commandprompt.com writes:


Here's an attempt to revive this patch.


Here's the patch rebased against current HEAD, that is including the
recently committed action_at_recovery_target option.

The default for the new GUC is 'pause', as in HEAD, and
pause_at_recovery_target is removed completely in favor of it.

I've also taken the liberty to remove that part that errors out when
finding $PGDATA/recovery.conf.  Now get your rotten tomatoes ready. ;-)


This was rather short-sighted, so I've restored that part.

Also, rebased on current HEAD, following the rename of
action_at_recovery_target to recovery_target_action.



Hi,

I had a quick look, the patch does not apply cleanly anymore but it's 
just release notes so nothing too bad.


I did not do any testing yet, but I have few comments about the code:

- the patch mixes functionality change with the lowercasing of the 
config variables, I wonder if those could be separated into 2 separate 
diffs - it would make review somewhat easier, but I can live with it as 
it is if it's too much work do separate into 2 patches


- the StandbyModeRequested and StandbyMode should be lowercased like the 
rest of the GUCs


- I am wondering if StandbyMode and hot_standby should be merged into 
one GUC if we are breaking backwards compatibility anyway


- Could you explain the reasoning behind:
+   if ((*newval)[0] == 0)
+   xid = InvalidTransactionId;
+   else

in check_recovery_target_xid() (and same check in 
check_recovery_target_timeline()), isn't the strtoul which is called 
later enough?


- The whole CopyErrorData and memory context logic is not needed in 
check_recovery_target_time() as the FlushErrorState() is not called there


- The new code in StartupXLOG() like
+   if (recovery_target_xid_string != NULL)
+   InitRecoveryTarget(RECOVERY_TARGET_XID);
+
+   if (recovery_target_time_string != NULL)
+   InitRecoveryTarget(RECOVERY_TARGET_TIME);
+
+   if (recovery_target_name != NULL)
+   InitRecoveryTarget(RECOVERY_TARGET_NAME);
+
+   if (recovery_target_string != NULL)
+   InitRecoveryTarget(RECOVERY_TARGET_IMMEDIATE);

would probably be better in separate function that you then call 
StartupXLOG() as StartupXLOG() is crazy long already so I think it's 
preferable to not make it worse.


- I wonder if we should rename trigger_file to standby_tigger_file


--
 Petr Jelinek  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


Re: [HACKERS] Turning recovery.conf into GUCs

2014-12-12 Thread Alex Shulgin
Alex Shulgin a...@commandprompt.com writes:

 Alex Shulgin a...@commandprompt.com writes:

 Here's an attempt to revive this patch.

 Here's the patch rebased against current HEAD, that is including the
 recently committed action_at_recovery_target option.

 The default for the new GUC is 'pause', as in HEAD, and
 pause_at_recovery_target is removed completely in favor of it.

 I've also taken the liberty to remove that part that errors out when
 finding $PGDATA/recovery.conf.  Now get your rotten tomatoes ready. ;-)

This was rather short-sighted, so I've restored that part.

Also, rebased on current HEAD, following the rename of
action_at_recovery_target to recovery_target_action.

--
Alex



recovery_guc_v5.5.patch.gz
Description: application/gzip

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


Re: [HACKERS] Turning recovery.conf into GUCs

2014-12-02 Thread Alex Shulgin

Michael Paquier michael.paqu...@gmail.com writes:

 On Tue, Dec 2, 2014 at 1:58 AM, Alex Shulgin a...@commandprompt.com wrote:
 Here's the patch rebased against current HEAD, that is including the
 recently committed action_at_recovery_target option.
 If this patch gets in, it gives a good argument to jump to 10.0 IMO.
 That's not a bad thing, only the cost of making recovery params as
 GUCs which is still a feature wanted.

 The default for the new GUC is 'pause', as in HEAD, and
 pause_at_recovery_target is removed completely in favor of it.
 Makes sense. Another idea that popped out was to rename this parameter
 as recovery_target_action as well, but that's not really something
 this patch should care about.

Indeed, but changing the name after the fact is straightforward.

 I've also taken the liberty to remove that part that errors out when
 finding $PGDATA/recovery.conf.
 I am not in favor of this part. It may be better to let the users know
 that their old configuration is not valid anymore with an error. This
 patch cuts in the flesh with a huge axe, let's be sure that users do
 not ignore the side pain effects, or recovery.conf would be simply
 ignored and users would not be aware of that.

Yeah, that is good point.

I'd be in favor of a solution that works the same way as before the
patch, without the need for extra trigger files, etc., but that doesn't
seem to be nearly possible.  Whatever tricks we might employ will likely
be defeated by the fact that the oldschool user will fail to *include*
recovery.conf in the main conf file.

--
Alex


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


Re: [HACKERS] Turning recovery.conf into GUCs

2014-12-02 Thread Josh Berkus
On 12/02/2014 06:25 AM, Alex Shulgin wrote:
 I am not in favor of this part. It may be better to let the users know
  that their old configuration is not valid anymore with an error. This
  patch cuts in the flesh with a huge axe, let's be sure that users do
  not ignore the side pain effects, or recovery.conf would be simply
  ignored and users would not be aware of that.
 Yeah, that is good point.
 
 I'd be in favor of a solution that works the same way as before the
 patch, without the need for extra trigger files, etc., but that doesn't
 seem to be nearly possible.  

As previously discussed, there are ways to avoid having a trigger file
for replication.  However, it's hard to avoid having one for PITR
recovery; at least, I can't think of a method which isn't just as
awkward, and we might as well stick with the awkward method we know.

 Whatever tricks we might employ will likely
 be defeated by the fact that the oldschool user will fail to *include*
 recovery.conf in the main conf file.

Well, can we merge this patch and then fight out what to do for the
transitional users as a separate patch?

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


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


Re: [HACKERS] Turning recovery.conf into GUCs

2014-12-02 Thread Alvaro Herrera
Josh Berkus wrote:
 On 12/02/2014 06:25 AM, Alex Shulgin wrote:

  Whatever tricks we might employ will likely
  be defeated by the fact that the oldschool user will fail to *include*
  recovery.conf in the main conf file.
 
 Well, can we merge this patch and then fight out what to do for the
 transitional users as a separate patch?

You seem to be saying I don't have any good idea how to solve this
problem now, but I will magically have one once this is committed.  I'm
not sure that works very well.

In any case, the proposal upthread that we raise an error if
recovery.conf is found seems sensible enough.  Users will see it and
they will adjust their stuff -- it's a one-time thing.  It's not like
they switch a version forwards one week and back the following week.

-- 
Álvaro Herrerahttp://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


Re: [HACKERS] Turning recovery.conf into GUCs

2014-12-02 Thread Josh Berkus
On 12/02/2014 10:31 AM, Alvaro Herrera wrote:
 Josh Berkus wrote:
 On 12/02/2014 06:25 AM, Alex Shulgin wrote:
 
 Whatever tricks we might employ will likely
 be defeated by the fact that the oldschool user will fail to *include*
 recovery.conf in the main conf file.

 Well, can we merge this patch and then fight out what to do for the
 transitional users as a separate patch?
 
 You seem to be saying I don't have any good idea how to solve this
 problem now, but I will magically have one once this is committed.  I'm
 not sure that works very well.

No, I'm saying this problem is easy to solve technically, but we have
intractable arguments on this list about the best way to solve it, even
though the bulk of the patch isn't in dispute.

 In any case, the proposal upthread that we raise an error if
 recovery.conf is found seems sensible enough.  Users will see it and
 they will adjust their stuff -- it's a one-time thing.  It's not like
 they switch a version forwards one week and back the following week.

I'm OK with that solution.  Apparently others aren't though.

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


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


Re: [HACKERS] Turning recovery.conf into GUCs

2014-12-02 Thread Andres Freund
On 2014-12-02 17:25:14 +0300, Alex Shulgin wrote:
 I'd be in favor of a solution that works the same way as before the
 patch, without the need for extra trigger files, etc., but that doesn't
 seem to be nearly possible.  Whatever tricks we might employ will likely
 be defeated by the fact that the oldschool user will fail to *include*
 recovery.conf in the main conf file.

I think removing the ability to define a trigger file is pretty much
unacceptable. It's not *too* bad to rewrite recovery.conf's contents
into actual valid postgresql.conf format, but it's harder to change an
existing complex failover setup that relies on the existance of such a
trigger.  I think aiming for removal of that is a sure way to prevent
the patch from getting in.

Greetings,

Andres Freund

-- 
 Andres Freund 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


Re: [HACKERS] Turning recovery.conf into GUCs

2014-12-02 Thread Alex Shulgin

Andres Freund and...@2ndquadrant.com writes:

 On 2014-12-02 17:25:14 +0300, Alex Shulgin wrote:
 I'd be in favor of a solution that works the same way as before the
 patch, without the need for extra trigger files, etc., but that doesn't
 seem to be nearly possible.  Whatever tricks we might employ will likely
 be defeated by the fact that the oldschool user will fail to *include*
 recovery.conf in the main conf file.

 I think removing the ability to define a trigger file is pretty much
 unacceptable. It's not *too* bad to rewrite recovery.conf's contents
 into actual valid postgresql.conf format, but it's harder to change an
 existing complex failover setup that relies on the existance of such a
 trigger.  I think aiming for removal of that is a sure way to prevent
 the patch from getting in.

To make it clear, I was talking not about trigger_file, but about
standby.enabled file which triggers the recovery/pitr/standby scenario
in the current form of the patch and stands as a replacement check
instead of the original check for the presense of recovery.conf.

--
Alex


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


Re: [HACKERS] Turning recovery.conf into GUCs

2014-12-01 Thread Alex Shulgin
Alex Shulgin a...@commandprompt.com writes:

 Here's an attempt to revive this patch.

Here's the patch rebased against current HEAD, that is including the
recently committed action_at_recovery_target option.

The default for the new GUC is 'pause', as in HEAD, and
pause_at_recovery_target is removed completely in favor of it.

I've also taken the liberty to remove that part that errors out when
finding $PGDATA/recovery.conf.  Now get your rotten tomatoes ready. ;-)

--
Alex



recovery_guc_v5.4.patch.gz
Description: application/gzip

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


Re: [HACKERS] Turning recovery.conf into GUCs

2014-12-01 Thread Michael Paquier
On Tue, Dec 2, 2014 at 1:58 AM, Alex Shulgin a...@commandprompt.com wrote:
 Here's the patch rebased against current HEAD, that is including the
 recently committed action_at_recovery_target option.
If this patch gets in, it gives a good argument to jump to 10.0 IMO.
That's not a bad thing, only the cost of making recovery params as
GUCs which is still a feature wanted.

 The default for the new GUC is 'pause', as in HEAD, and
 pause_at_recovery_target is removed completely in favor of it.
Makes sense. Another idea that popped out was to rename this parameter
as recovery_target_action as well, but that's not really something
this patch should care about.

 I've also taken the liberty to remove that part that errors out when
 finding $PGDATA/recovery.conf.
I am not in favor of this part. It may be better to let the users know
that their old configuration is not valid anymore with an error. This
patch cuts in the flesh with a huge axe, let's be sure that users do
not ignore the side pain effects, or recovery.conf would be simply
ignored and users would not be aware of that.
Regards,
-- 
Michael


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


Re: [HACKERS] Turning recovery.conf into GUCs

2014-11-24 Thread Alex Shulgin

Alex Shulgin a...@commandprompt.com writes:

  * Why do you include xlog_internal.h in guc.c and not xlog.h?

 we actually need both but including xlog_internal.h also includes xlog.h
 i added xlog.h and if someone things is enough only putting
 xlog_internal.h let me know

 What's required from xlog_internal.h?

 Looks like this was addressed before me.

Actually, it's there to bring in MAXFNAMELEN which is used in
check_recovery_target_name.  Not sure how it even compiled without that,
but after some branch switching it doesn't anymore. :-p

Maybe we should move these check/assign hooks to xlog.c, but that's
likely going to create header files dependency problem due to use of
GucSource in the hook prototype...

--
Alex


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


Re: [HACKERS] Turning recovery.conf into GUCs

2014-11-24 Thread Alex Shulgin

Josh Berkus j...@agliodbs.com writes:
 
 Well, the latest version of this patch fails to start when it sees
 'recovery.conf' in PGDATA:
 
   FATAL:  recovery.conf is not supported anymore as a recovery method
   DETAIL:  Refer to appropriate documentation about migration methods
 
 I've missed all the discussion behind this decision and after reading
 the ALTER SYSTEM/conf.d thread I'm even more confused so I'd like
 someone more knowledgeable to speak up on the status of this.

 The argument was that people wanted to be able to have an empty
 recovery.conf file as a we're in standby trigger so that they could
 preserve backwards compatibility with external tools.  I don't agree
 with this argument, but several people championed it.

It doesn't look like we can provide for 100% backwards compatibility
with existing tools, here's why:

a) The only sensible way to use recovery.conf as GUC source is via
   include/include_dir from postgresql.conf.

b) The server used to rename $PGDATA/recovery.conf to
   $PGDATA/recovery.done so when it is re-started it doesn't
   accidentally start in recovery mode.

We can't keep doing (b) because that will make postgres fail to start on
a missing include.  Well, it won't error out if we place the file under
include_dir, as only files with the .conf suffix are included, but
then again the server wouldn't know which file to rename unless we tell
it or hard-code the the filename.  Either way this is not 100% backwards
compatible.

Now the question is if we are going to break compatibility anyway:
should we try to minimize the difference or will it disserve the user by
providing a false sense of compatibility?

For one, if we're breaking things, the trigger_file GUC should be
renamed to end_recovery_trigger_file or something like that, IMO.
That's if we decide to keep it at all.

 The docs use the term continuous recovery.
 
 Either way, from the code it is clear that we only stay in recovery if
 standby_mode is directly turned on.  This makes the whole check for a
 specially named file unnecessary, IMO: we should just check the value of
 standby_mode (which is off by default).

 So, what happens when someone does pg_ctl promote?  Somehow
 standby_mode needs to get set to off.  Maybe we write standby_mode =
 off to postgresql.auto.conf?

Well, standby_mode is only consulted at startup after crash recovery.
But suddenly, a tool that *writes* recovery.conf needs to also
consult/edit postgresql.auto.conf...  Maybe that's inevitable, but still
a point to consider.

 By the way, is there any use in setting standby_mode=on and any of the
 recovery_target* GUCs at the same time?

 See my thread on this topic from last week.  Short answer: No.

 I think it can only play together if you set the target farther than the
 latest point you've got in the archive locally.  So that's sort of
 Point-in-Future-Recovery.  Does that make any sense at all?

 Again, see the thread.  This doesn't work in a useful way, so there's no
 reason to write code to enable it.

Makes sense.  Should we incorporate the actual tech and doc fix in this
patch?

  /* File path names (all relative to $PGDATA) */
 -#define RECOVERY_COMMAND_FILE recovery.conf
 -#define RECOVERY_COMMAND_DONE recovery.done
 +#define RECOVERY_ENABLE_FILE  standby.enabled

 Imo the file and variable names should stay coherent.
 
 Yes, once we settle on the name (and if we really need that extra
 trigger file.)

 Personally, if we have three methods of promotion:

 1) pg_ctl promote
 2) edit postgresql.conf and reload
 3) ALTER SYSTEM SET and reload

 ... I don't honestly think we need a 4th method for promotion.

Me neither.  It is tempting to make everything spin around GUCs without
the need for any external trigger files.

 The main reason to want a we're in recovery file is for PITR rather
 than for replication, where it has a number of advantages as a method,
 the main one being that recovery.conf is unlikely to be overwritten by
 the contents of the backup.

 HOWEVER, there's a clear out for this with conf.d.  If we enable conf.d
 by default, then we can simply put recovery settings in conf.d, and
 since that specific file (which can have whatever name the user chooses)
 will not be part of backups, it would have the same advantage as
 recovery.conf, without the drawbacks.

 Discuss?

Well, I don't readily see how conf.d is special with regard to base
backup, wouldn't you need to exclude it explicitly still?

--
Alex


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


Re: [HACKERS] Turning recovery.conf into GUCs

2014-11-24 Thread Stephen Frost
* Amit Kapila (amit.kapil...@gmail.com) wrote:
 What exactly you mean by 'disable postgresql.auto.conf',  do you
 mean user runs Alter System to remove that entry or manually disable
 some particular entry?

Last I paid attention to this, there was a clear way to disable the
inclusion of postgresql.auto.conf in the postgresql.conf.  If that's
gone, then there is a serious problem.  Administrators who manage their
postgresql.conf (eg: through a CM system like puppet or chef..) must
have a way to prevent other changes.

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] Turning recovery.conf into GUCs

2014-11-24 Thread Alvaro Herrera
Stephen Frost wrote:
 * Amit Kapila (amit.kapil...@gmail.com) wrote:
  What exactly you mean by 'disable postgresql.auto.conf',  do you
  mean user runs Alter System to remove that entry or manually disable
  some particular entry?
 
 Last I paid attention to this, there was a clear way to disable the
 inclusion of postgresql.auto.conf in the postgresql.conf.  If that's
 gone, then there is a serious problem.  Administrators who manage their
 postgresql.conf (eg: through a CM system like puppet or chef..) must
 have a way to prevent other changes.

Sigh, here we go again.  I don't think you can disable postgresql.auto.conf
in the current code.  As I recall, the argument was that it's harder to
diagnose problems if postgresql.auto.conf takes effect in some system
but not others.

I think if you want puppet or chef etc you'd add postgresql.auto.conf as
a config file in those systems, so that ALTER SYSTEM is reflected there.  

-- 
Álvaro Herrerahttp://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


Re: [HACKERS] Turning recovery.conf into GUCs

2014-11-24 Thread Stephen Frost
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
 Stephen Frost wrote:
  * Amit Kapila (amit.kapil...@gmail.com) wrote:
   What exactly you mean by 'disable postgresql.auto.conf',  do you
   mean user runs Alter System to remove that entry or manually disable
   some particular entry?
  
  Last I paid attention to this, there was a clear way to disable the
  inclusion of postgresql.auto.conf in the postgresql.conf.  If that's
  gone, then there is a serious problem.  Administrators who manage their
  postgresql.conf (eg: through a CM system like puppet or chef..) must
  have a way to prevent other changes.
 
 Sigh, here we go again.  I don't think you can disable postgresql.auto.conf
 in the current code.  As I recall, the argument was that it's harder to
 diagnose problems if postgresql.auto.conf takes effect in some system
 but not others.

I don't buy this at all.  What's going to be terribly confusing is to
have config changes start happening for users who are managing their
configs through a CM (which most should be..).  ALTER SYSTEM is going to
cause more problems than it solves.

 I think if you want puppet or chef etc you'd add postgresql.auto.conf as
 a config file in those systems, so that ALTER SYSTEM is reflected there.  

That's really a horrible, horrible answer.  The DBA makes some change
and then reloads remotely, only to have puppet or chef come along and
change it back later?  Talk about a recipe for disaster.

The only reason I stopped worrying about the foolishness of ALTER SYSTEM
was because it could be disabled.  I'm very disappointed to hear that
someone saw fit to remove that.  I'll also predict that it'll be going
back in for 9.5.

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] Turning recovery.conf into GUCs

2014-11-24 Thread Alvaro Herrera
Stephen Frost wrote:
 * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:

   Last I paid attention to this, there was a clear way to disable the
   inclusion of postgresql.auto.conf in the postgresql.conf.  If that's
   gone, then there is a serious problem.  Administrators who manage their
   postgresql.conf (eg: through a CM system like puppet or chef..) must
   have a way to prevent other changes.
  
  Sigh, here we go again.  I don't think you can disable postgresql.auto.conf
  in the current code.  As I recall, the argument was that it's harder to
  diagnose problems if postgresql.auto.conf takes effect in some system
  but not others.
 
 I don't buy this at all.  What's going to be terribly confusing is to
 have config changes start happening for users who are managing their
 configs through a CM (which most should be..).  ALTER SYSTEM is going to
 cause more problems than it solves.

I guess if you have two DBAs who don't talk to each other, and one
changes things through puppet and another through ALTER SYSTEM, it's
going to be confusing, yes.

  I think if you want puppet or chef etc you'd add postgresql.auto.conf as
  a config file in those systems, so that ALTER SYSTEM is reflected there.  
 
 That's really a horrible, horrible answer.  The DBA makes some change
 and then reloads remotely, only to have puppet or chef come along and
 change it back later?  Talk about a recipe for disaster.

Are you saying puppet/chef don't have the concept that a file is to be
backed up and changes on it notified, but that direct changes to it
should not be allowed?  That sounds, um, limited.

 The only reason I stopped worrying about the foolishness of ALTER SYSTEM
 was because it could be disabled.  I'm very disappointed to hear that
 someone saw fit to remove that.  I'll also predict that it'll be going
 back in for 9.5.

*shrug*

-- 
Álvaro Herrerahttp://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


Re: [HACKERS] Turning recovery.conf into GUCs

2014-11-24 Thread Stephen Frost
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
 Stephen Frost wrote:
  * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
 
Last I paid attention to this, there was a clear way to disable the
inclusion of postgresql.auto.conf in the postgresql.conf.  If that's
gone, then there is a serious problem.  Administrators who manage their
postgresql.conf (eg: through a CM system like puppet or chef..) must
have a way to prevent other changes.
   
   Sigh, here we go again.  I don't think you can disable 
   postgresql.auto.conf
   in the current code.  As I recall, the argument was that it's harder to
   diagnose problems if postgresql.auto.conf takes effect in some system
   but not others.
  
  I don't buy this at all.  What's going to be terribly confusing is to
  have config changes start happening for users who are managing their
  configs through a CM (which most should be..).  ALTER SYSTEM is going to
  cause more problems than it solves.
 
 I guess if you have two DBAs who don't talk to each other, and one
 changes things through puppet and another through ALTER SYSTEM, it's
 going to be confusing, yes.

It's not DBAs, that's the point..  You have sysadmins who manage the
system configs (things like postgresql.conf) and you have DBAs whose
only access to the system is through 5432.  This seperation of
responsibilities is very common, in my experience at least, and
conflating the two through ALTER SYSTEM is going to cause nothing but
problems.  There had been a way to keep that seperation by simply
disabling the postgresql.auto.conf, but that's now been removed.

   I think if you want puppet or chef etc you'd add postgresql.auto.conf as
   a config file in those systems, so that ALTER SYSTEM is reflected there.  
  
  That's really a horrible, horrible answer.  The DBA makes some change
  and then reloads remotely, only to have puppet or chef come along and
  change it back later?  Talk about a recipe for disaster.
 
 Are you saying puppet/chef don't have the concept that a file is to be
 backed up and changes on it notified, but that direct changes to it
 should not be allowed?  That sounds, um, limited.

Of course they can but that's completely missing the point.  The
postgresql.conf file is *already* managed in puppet or chef in a *lot*
of places.  We're removing the ability to do that and reverting to a
situation where auditing has to be done instead.  That's a regression,
not a step forward.

  The only reason I stopped worrying about the foolishness of ALTER SYSTEM
  was because it could be disabled.  I'm very disappointed to hear that
  someone saw fit to remove that.  I'll also predict that it'll be going
  back in for 9.5.
 
 *shrug*

... and I'll continue to argue against anything which requires
postgresql.auto.conf to be hacked to work (as was proposed on this
thread..).

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] Turning recovery.conf into GUCs

2014-11-24 Thread Andres Freund
On 2014-11-24 10:13:58 -0500, Stephen Frost wrote:
 * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
  Stephen Frost wrote:
   * Amit Kapila (amit.kapil...@gmail.com) wrote:
What exactly you mean by 'disable postgresql.auto.conf',  do you
mean user runs Alter System to remove that entry or manually disable
some particular entry?
   
   Last I paid attention to this, there was a clear way to disable the
   inclusion of postgresql.auto.conf in the postgresql.conf.  If that's
   gone, then there is a serious problem.  Administrators who manage their
   postgresql.conf (eg: through a CM system like puppet or chef..) must
   have a way to prevent other changes.
  
  Sigh, here we go again.  I don't think you can disable postgresql.auto.conf
  in the current code.  As I recall, the argument was that it's harder to
  diagnose problems if postgresql.auto.conf takes effect in some system
  but not others.
 
 I don't buy this at all.  What's going to be terribly confusing is to
 have config changes start happening for users who are managing their
 configs through a CM (which most should be..).  ALTER SYSTEM is going to
 cause more problems than it solves.

I fail to see how this really has really anything to do with this
topic. Obviously ALTER SYSTEM isn't a applicable solution for this as HS
might not be in use or HS might not have reached consistency at that
point.

Greetings,

Andres Freund

-- 
 Andres Freund 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


Re: [HACKERS] Turning recovery.conf into GUCs

2014-11-24 Thread Tom Lane
Alex Shulgin a...@commandprompt.com writes:
 Maybe we should move these check/assign hooks to xlog.c, but that's
 likely going to create header files dependency problem due to use of
 GucSource in the hook prototype...

As far as that goes, there is already plenty of precedent for declaring
assorted check/assign hook functions in guc.h rather than including guc.h
into the headers where they would otherwise belong.

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


Re: [HACKERS] Turning recovery.conf into GUCs

2014-11-24 Thread David G Johnston
Stephen Frost wrote
 * Alvaro Herrera (

 alvherre@

 ) wrote:
 Stephen Frost wrote:
  * Amit Kapila (

 amit.kapila16@

 ) wrote:
   What exactly you mean by 'disable postgresql.auto.conf',  do you
   mean user runs Alter System to remove that entry or manually disable
   some particular entry?
 
 Sigh, here we go again.  I don't think you can disable
 postgresql.auto.conf
 in the current code.  As I recall, the argument was that it's harder to
 diagnose problems if postgresql.auto.conf takes effect in some system
 but not others.
 
 I don't buy this at all.  What's going to be terribly confusing is to
 have config changes start happening for users who are managing their
 configs through a CM (which most should be..).  ALTER SYSTEM is going to
 cause more problems than it solves.

So what happens if someone makes postgresql.auto.conf read-only (to
everyone)?

David J.




--
View this message in context: 
http://postgresql.nabble.com/Turning-recovery-conf-into-GUCs-tp5774757p5828052.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.


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


Re: [HACKERS] Turning recovery.conf into GUCs

2014-11-24 Thread Jaime Casanova
On Fri, Nov 21, 2014 at 12:55 PM, Josh Berkus j...@agliodbs.com wrote:
 On 11/21/2014 09:35 AM, Alex Shulgin wrote:
 Hello,

 Here's an attempt to revive this patch.

 Yayy!  Thank you.

 People might not like me for the suggestion, but I think we should
 simply always include a 'recovery.conf' in $PGDATA
 unconditionally. That'd make this easier.
 Alternatively we could pass a filename to --write-recovery-conf.

 Well, the latest version of this patch fails to start when it sees
 'recovery.conf' in PGDATA:

   FATAL:  recovery.conf is not supported anymore as a recovery method
   DETAIL:  Refer to appropriate documentation about migration methods

 I've missed all the discussion behind this decision and after reading
 the ALTER SYSTEM/conf.d thread I'm even more confused so I'd like
 someone more knowledgeable to speak up on the status of this.

 The argument was that people wanted to be able to have an empty
 recovery.conf file as a we're in standby trigger so that they could
 preserve backwards compatibility with external tools.  I don't agree
 with this argument, but several people championed it.


well, my approach was that postgres just ignore the file completely. I
mean, recovery.conf will no longer mean anything special.
Then, every tool that create recovery.conf in $PGDATA only has to add
an ALTER SYSTEM to include it

 The docs use the term continuous recovery.

 Either way, from the code it is clear that we only stay in recovery if
 standby_mode is directly turned on.  This makes the whole check for a
 specially named file unnecessary, IMO: we should just check the value of
 standby_mode (which is off by default).


no. currently we enter in recovery mode when postgres see a
recovery.conf and stays in recovery mode when standby_mode is on or an
appropiate restore_command is provided.

which means recovery.conf has two uses:
1) start in recovery mode (not continuous)
2) provide parameters for recovery mode and for streaming

we still need a recovery trigger file that forces postgres to start
in recovery mode and acts accordingly to recovery GUCs

 So, what happens when someone does pg_ctl promote?  Somehow
 standby_mode needs to get set to off.  Maybe we write standby_mode =
 off to postgresql.auto.conf?


we need to delete or rename the recovery trigger file, all standby
GUCs are ignored (and recovery GUCs should be ignored too) unless
you're in recovery mode

 By the way, is there any use in setting standby_mode=on and any of the
 recovery_target* GUCs at the same time?

 See my thread on this topic from last week.  Short answer: No.


haven't read that thread, will do


  /* File path names (all relative to $PGDATA) */
 -#define RECOVERY_COMMAND_FILE  recovery.conf
 -#define RECOVERY_COMMAND_DONE  recovery.done
 +#define RECOVERY_ENABLE_FILE   standby.enabled

 Imo the file and variable names should stay coherent.

 Yes, once we settle on the name (and if we really need that extra
 trigger file.)


yes, we need it. but other names were suggested standby.enabled
transmit the wrong idea

 Personally, if we have three methods of promotion:

 1) pg_ctl promote
 2) edit postgresql.conf and reload
 3) ALTER SYSTEM SET and reload

 ... I don't honestly think we need a 4th method for promotion.


this is not for promotion, this is to force postgres to start in
recovery mode and read recovery configuration parameters.

 The main reason to want a we're in recovery file is for PITR rather
 than for replication, where it has a number of advantages as a method,
 the main one being that recovery.conf is unlikely to be overwritten by
 the contents of the backup.


only that you need to start in recovery mode to start replication

-- 
Jaime Casanova www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación
Phone: +593 4 5107566 Cell: +593 987171157


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


Re: [HACKERS] Turning recovery.conf into GUCs

2014-11-24 Thread Jaime Casanova
On Mon, Nov 24, 2014 at 12:24 PM, Jaime Casanova ja...@2ndquadrant.com wrote:
 On Fri, Nov 21, 2014 at 12:55 PM, Josh Berkus j...@agliodbs.com wrote:
 On 11/21/2014 09:35 AM, Alex Shulgin wrote:
 Hello,

 Here's an attempt to revive this patch.

 Yayy!  Thank you.

 People might not like me for the suggestion, but I think we should
 simply always include a 'recovery.conf' in $PGDATA
 unconditionally. That'd make this easier.
 Alternatively we could pass a filename to --write-recovery-conf.

 Well, the latest version of this patch fails to start when it sees
 'recovery.conf' in PGDATA:

   FATAL:  recovery.conf is not supported anymore as a recovery method
   DETAIL:  Refer to appropriate documentation about migration methods

 I've missed all the discussion behind this decision and after reading
 the ALTER SYSTEM/conf.d thread I'm even more confused so I'd like
 someone more knowledgeable to speak up on the status of this.

 The argument was that people wanted to be able to have an empty
 recovery.conf file as a we're in standby trigger so that they could
 preserve backwards compatibility with external tools.  I don't agree
 with this argument, but several people championed it.


 well, my approach was that postgres just ignore the file completely. I
 mean, recovery.conf will no longer mean anything special.
 Then, every tool that create recovery.conf in $PGDATA only has to add
 an ALTER SYSTEM to include it


i mean ALTER SYSTEM in master before copying or just add the line in
postgresql.conf

but, as the patch shows agreement was to break backwards compatibility
and fail to start if the file is present

-- 
Jaime Casanova www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación
Phone: +593 4 5107566 Cell: +593 987171157


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


Re: [HACKERS] Turning recovery.conf into GUCs

2014-11-24 Thread Josh Berkus
On 11/24/2014 09:24 AM, Jaime Casanova wrote:
 ... I don't honestly think we need a 4th method for promotion.

 
 this is not for promotion, this is to force postgres to start in
 recovery mode and read recovery configuration parameters.
 
 The main reason to want a we're in recovery file is for PITR rather
 than for replication, where it has a number of advantages as a method,
 the main one being that recovery.conf is unlikely to be overwritten by
 the contents of the backup.

 
 only that you need to start in recovery mode to start replication

Right, but my point is that having a trigger file *is not necessary for
replication, only for PITR* -- and maybe not necessary even for PITR.
That is, in a streaming replication configuration, having a
standby_mode = on|off parameter is 100% sufficient to control
replication with the small detail that pg_ctl promote needs to set it
in pg.auto.conf or conf.d.

And, now, having given it some thought, I'm going to argue that it's not
required for PITR either, provided that we can use the auto.conf method.

Before I go into my ideas, though, what does the current patch do
regarding non-replication PITR?

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


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


Re: [HACKERS] Turning recovery.conf into GUCs

2014-11-24 Thread Alex Shulgin

Jaime Casanova ja...@2ndquadrant.com writes:

 Either way, from the code it is clear that we only stay in recovery if
 standby_mode is directly turned on.  This makes the whole check for a
 specially named file unnecessary, IMO: we should just check the value of
 standby_mode (which is off by default).

 no. currently we enter in recovery mode when postgres see a
 recovery.conf and stays in recovery mode when standby_mode is on or an
 appropiate restore_command is provided.

 which means recovery.conf has two uses:
 1) start in recovery mode (not continuous)
 2) provide parameters for recovery mode and for streaming

 we still need a recovery trigger file that forces postgres to start
 in recovery mode and acts accordingly to recovery GUCs

Yes, these were my latest findings also, but if instead of removing the
trigger file upon successful recovery the server would set
standby_mode=off, that would also work.

--
Alex


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


Re: [HACKERS] Turning recovery.conf into GUCs

2014-11-24 Thread Alex Shulgin

Josh Berkus j...@agliodbs.com writes:
 
 only that you need to start in recovery mode to start replication

 Right, but my point is that having a trigger file *is not necessary for
 replication, only for PITR* -- and maybe not necessary even for PITR.
 That is, in a streaming replication configuration, having a
 standby_mode = on|off parameter is 100% sufficient to control
 replication with the small detail that pg_ctl promote needs to set it
 in pg.auto.conf or conf.d.

 And, now, having given it some thought, I'm going to argue that it's not
 required for PITR either, provided that we can use the auto.conf method.

 Before I go into my ideas, though, what does the current patch do
 regarding non-replication PITR?

It removes that $PGDATA/standby.enable trigger file it relies on to
start the PITR in the first place.

--
Alex


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


Re: [HACKERS] Turning recovery.conf into GUCs

2014-11-24 Thread Andres Freund
On 2014-11-24 12:02:52 -0800, Josh Berkus wrote:
 On 11/24/2014 09:24 AM, Jaime Casanova wrote:
  ... I don't honestly think we need a 4th method for promotion.
 
  
  this is not for promotion, this is to force postgres to start in
  recovery mode and read recovery configuration parameters.
  
  The main reason to want a we're in recovery file is for PITR rather
  than for replication, where it has a number of advantages as a method,
  the main one being that recovery.conf is unlikely to be overwritten by
  the contents of the backup.
 
  
  only that you need to start in recovery mode to start replication
 
 Right, but my point is that having a trigger file *is not necessary for
 replication, only for PITR* -- and maybe not necessary even for PITR.
 That is, in a streaming replication configuration, having a
 standby_mode = on|off parameter is 100% sufficient to control
 replication with the small detail that pg_ctl promote needs to set it
 in pg.auto.conf or conf.d.
 
 And, now, having given it some thought, I'm going to argue that it's not
 required for PITR either, provided that we can use the auto.conf method.
 
 Before I go into my ideas, though, what does the current patch do
 regarding non-replication PITR?

Guys. We aren't rereading the GUCs in the relevant places.  It's also
decidedly nontrivial to make standby_mode PGC_SIGHUP. Don't make this
patch more complex than it has to be. That's what stalled it the last
times round.

Greetings,

Andres Freund

-- 
 Andres Freund 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


Re: [HACKERS] Turning recovery.conf into GUCs

2014-11-24 Thread Josh Berkus
On 11/24/2014 02:00 PM, Alex Shulgin wrote:
 
 Josh Berkus j...@agliodbs.com writes:

 only that you need to start in recovery mode to start replication

 Right, but my point is that having a trigger file *is not necessary for
 replication, only for PITR* -- and maybe not necessary even for PITR.
 That is, in a streaming replication configuration, having a
 standby_mode = on|off parameter is 100% sufficient to control
 replication with the small detail that pg_ctl promote needs to set it
 in pg.auto.conf or conf.d.

 And, now, having given it some thought, I'm going to argue that it's not
 required for PITR either, provided that we can use the auto.conf method.

 Before I go into my ideas, though, what does the current patch do
 regarding non-replication PITR?
 
 It removes that $PGDATA/standby.enable trigger file it relies on to
 start the PITR in the first place.

OK, and that's required for replication too?  I'm OK with that if it
gets the patch in.

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


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


Re: [HACKERS] Turning recovery.conf into GUCs

2014-11-24 Thread Alex Shulgin

Josh Berkus j...@agliodbs.com writes:

 Before I go into my ideas, though, what does the current patch do
 regarding non-replication PITR?
 
 It removes that $PGDATA/standby.enable trigger file it relies on to
 start the PITR in the first place.

 OK, and that's required for replication too?  I'm OK with that if it
 gets the patch in.

In the current form of the patch, yes.  Thought I don't think I like it.

--
Alex


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


Re: [HACKERS] Turning recovery.conf into GUCs

2014-11-24 Thread Josh Berkus
On 11/24/2014 02:18 PM, Alex Shulgin wrote:
 
 Josh Berkus j...@agliodbs.com writes:

 Before I go into my ideas, though, what does the current patch do
 regarding non-replication PITR?

 It removes that $PGDATA/standby.enable trigger file it relies on to
 start the PITR in the first place.

 OK, and that's required for replication too?  I'm OK with that if it
 gets the patch in.
 
 In the current form of the patch, yes.  Thought I don't think I like it.

One step at a time.


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


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


Re: [HACKERS] Turning recovery.conf into GUCs

2014-11-23 Thread Andres Freund
On 2014-11-21 10:59:23 -0800, Josh Berkus wrote:
 On 11/21/2014 10:54 AM, Stephen Frost wrote:
  * Josh Berkus (j...@agliodbs.com) wrote:
  Either way, from the code it is clear that we only stay in recovery if
  standby_mode is directly turned on.  This makes the whole check for a
  specially named file unnecessary, IMO: we should just check the value of
  standby_mode (which is off by default).
 
  So, what happens when someone does pg_ctl promote?  Somehow
  standby_mode needs to get set to off.  Maybe we write standby_mode =
  off to postgresql.auto.conf?
  
  Uhh, and then not work for the sane folks who disable
  postgresql.auto.conf?  No thanks..
 
 Other ideas then, without reverting to the old system?  Being able to
 promote servers over port 5432 will be a huge advantage for
 container-based systems, so I don't want to give that up as a feature.

Why is a trigger file making that impossible? Adding the code from
pg_ctl promote into a SQL callable function is a couple minutes worth of
work?

A guc alone won't work very well - standby_mode is checked in specific
places, you can't just turn that off and expect that that'd result in
speedy promotion. And it'd break people using scripts pg_standby. And it also
has other effects.

So, no.

Greetings,

Andres Freund

-- 
 Andres Freund 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


Re: [HACKERS] Turning recovery.conf into GUCs

2014-11-21 Thread Alex Shulgin
Hello,

Here's an attempt to revive this patch.

It is rebased onto the latest master and also includes handling and
documentation of newly added recovery.conf parameters such as
primary_slot_name, recovery_min_apply_delay and
recovery_target='immediate'.

The following feedback had been addressed:

Andres Freund and...@2ndquadrant.com writes:
 
  * --write-standby-enable seems to loose quite some functionality in
comparison to --write-recovery-conf since it doesn't seem to set
primary_conninfo, standby anymore.
 
 we can add code that looks for postgresql.conf in $PGDATA but if
 postgresql.conf is not there (like the case in debian, there is not
 much we can do about it) or we can write a file ready to be included
 in postgresql.conf, any sugestion?

 People might not like me for the suggestion, but I think we should
 simply always include a 'recovery.conf' in $PGDATA
 unconditionally. That'd make this easier.
 Alternatively we could pass a filename to --write-recovery-conf.

Well, the latest version of this patch fails to start when it sees
'recovery.conf' in PGDATA:

  FATAL:  recovery.conf is not supported anymore as a recovery method
  DETAIL:  Refer to appropriate documentation about migration methods

I've missed all the discussion behind this decision and after reading
the ALTER SYSTEM/conf.d thread I'm even more confused so I'd like
someone more knowledgeable to speak up on the status of this.

Do we want to keep this behavior of the patch?

  * CheckRecoveryReadyFile() doesn't seem to be a very descriptive
function name.
 
 I left it as CheckStartingAsStandby() but i still have a problem of
 this not being completely clear. this function is useful for standby
 or pitr.

There's not much left for this function in the current patch version, so
maybe we should just move it to StartupXLOG (it's not called from
anywhere else either way).

 which leads me to the other problem i have: the recovery trigger file,
 i have left it as standby.enabled but i still don't like it.

 recovery.trigger (Andres objected on this name)
 forced_recovery.trigger
 user_forced_recovery.trigger

 stay_in_recovery.trigger? That'd be pretty clear for anybody involved in
 pg, but otherwise...

The docs use the term continuous recovery.

Either way, from the code it is clear that we only stay in recovery if
standby_mode is directly turned on.  This makes the whole check for a
specially named file unnecessary, IMO: we should just check the value of
standby_mode (which is off by default).

By the way, is there any use in setting standby_mode=on and any of the
recovery_target* GUCs at the same time?

I think it can only play together if you set the target farther than the
latest point you've got in the archive locally.  So that's sort of
Point-in-Future-Recovery.  Does that make any sense at all?

  * the description of archive_cleanup_command seems wrong to me.
 
 why? it seems to be the same that was in recovery.conf. where did you
 see the description you're complaining at?

 I dislike the description in guc.c

 +{archive_cleanup_command, PGC_POSTMASTER, 
 WAL_ARCHIVE_RECOVERY,
 +gettext_noop(Sets the shell command that will be 
 executed at every restartpoint.),
 +NULL
 +},
 +archive_cleanup_command,

 previously it was:

 -# specifies an optional shell command to execute at every restartpoint.
 -# This can be useful for cleaning up the archive of a standby server.

Expanded the GUC desc.

  * Why the PG_TRY/PG_CATCH in check_recovery_target_time? Besides being
really strangely formatted (multiline :? inside a function?) it
doesn't a) seem to be correct to ignore potential memory allocation
errors by just switching back into the context that just errored out,
and continue to work there b) forgo cleanup by just continuing as if
nothing happened. That's unlikely to be acceptable.
 
 the code that read recovery.conf didn't has that, so i just removed it

 Well, that's not necessarily correct. recovery.conf was only read during
 startup, while this is read on SIGHUP.

[copied from the bottom, related]

 I don't think that's correct. Afaics it will cause the postmaster to
 crash on a SIGHUP with invalid data. I think you're unfortunately going
 to have to copy a fair bit of timestamptz_in() and even
 DateTimeParseError(), replacing the ereport()s by the guc error
 reporting.

The use of PG_TRY/CATCH does protect from FATALs in SIGHUP indeed.
Using CopyErrorData() we can also fetch the actual error message from
timestamptz_in, though I wonder we really have to make a full copy.

  * Why do you include xlog_internal.h in guc.c and not xlog.h?

 we actually need both but including xlog_internal.h also includes xlog.h
 i added xlog.h and if someone things is enough only putting
 xlog_internal.h let me know

 What's required from xlog_internal.h?

Looks like this was addressed before me.

 diff --git 

Re: [HACKERS] Turning recovery.conf into GUCs

2014-11-21 Thread Josh Berkus
On 11/21/2014 09:35 AM, Alex Shulgin wrote:
 Hello,
 
 Here's an attempt to revive this patch.

Yayy!  Thank you.

 People might not like me for the suggestion, but I think we should
 simply always include a 'recovery.conf' in $PGDATA
 unconditionally. That'd make this easier.
 Alternatively we could pass a filename to --write-recovery-conf.
 
 Well, the latest version of this patch fails to start when it sees
 'recovery.conf' in PGDATA:
 
   FATAL:  recovery.conf is not supported anymore as a recovery method
   DETAIL:  Refer to appropriate documentation about migration methods
 
 I've missed all the discussion behind this decision and after reading
 the ALTER SYSTEM/conf.d thread I'm even more confused so I'd like
 someone more knowledgeable to speak up on the status of this.

The argument was that people wanted to be able to have an empty
recovery.conf file as a we're in standby trigger so that they could
preserve backwards compatibility with external tools.  I don't agree
with this argument, but several people championed it.

 The docs use the term continuous recovery.
 
 Either way, from the code it is clear that we only stay in recovery if
 standby_mode is directly turned on.  This makes the whole check for a
 specially named file unnecessary, IMO: we should just check the value of
 standby_mode (which is off by default).

So, what happens when someone does pg_ctl promote?  Somehow
standby_mode needs to get set to off.  Maybe we write standby_mode =
off to postgresql.auto.conf?

 By the way, is there any use in setting standby_mode=on and any of the
 recovery_target* GUCs at the same time?

See my thread on this topic from last week.  Short answer: No.

 I think it can only play together if you set the target farther than the
 latest point you've got in the archive locally.  So that's sort of
 Point-in-Future-Recovery.  Does that make any sense at all?

Again, see the thread.  This doesn't work in a useful way, so there's no
reason to write code to enable it.

  /* File path names (all relative to $PGDATA) */
 -#define RECOVERY_COMMAND_FILE  recovery.conf
 -#define RECOVERY_COMMAND_DONE  recovery.done
 +#define RECOVERY_ENABLE_FILE   standby.enabled

 Imo the file and variable names should stay coherent.
 
 Yes, once we settle on the name (and if we really need that extra
 trigger file.)

Personally, if we have three methods of promotion:

1) pg_ctl promote
2) edit postgresql.conf and reload
3) ALTER SYSTEM SET and reload

... I don't honestly think we need a 4th method for promotion.

The main reason to want a we're in recovery file is for PITR rather
than for replication, where it has a number of advantages as a method,
the main one being that recovery.conf is unlikely to be overwritten by
the contents of the backup.

HOWEVER, there's a clear out for this with conf.d.  If we enable conf.d
by default, then we can simply put recovery settings in conf.d, and
since that specific file (which can have whatever name the user chooses)
will not be part of backups, it would have the same advantage as
recovery.conf, without the drawbacks.

Discuss?

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


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


Re: [HACKERS] Turning recovery.conf into GUCs

2014-11-21 Thread Stephen Frost
* Josh Berkus (j...@agliodbs.com) wrote:
  Either way, from the code it is clear that we only stay in recovery if
  standby_mode is directly turned on.  This makes the whole check for a
  specially named file unnecessary, IMO: we should just check the value of
  standby_mode (which is off by default).
 
 So, what happens when someone does pg_ctl promote?  Somehow
 standby_mode needs to get set to off.  Maybe we write standby_mode =
 off to postgresql.auto.conf?

Uhh, and then not work for the sane folks who disable
postgresql.auto.conf?  No thanks..

 HOWEVER, there's a clear out for this with conf.d.  If we enable conf.d
 by default, then we can simply put recovery settings in conf.d, and
 since that specific file (which can have whatever name the user chooses)
 will not be part of backups, it would have the same advantage as
 recovery.conf, without the drawbacks.

conf.d is a possibility, but there may be environments where the
postgres users doesn't have access to write into the conf.d directrory..
Not sure if that'd be an issue for what you're suggesting but wanted to
point it out.

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] Turning recovery.conf into GUCs

2014-11-21 Thread Josh Berkus
On 11/21/2014 10:54 AM, Stephen Frost wrote:
 * Josh Berkus (j...@agliodbs.com) wrote:
 Either way, from the code it is clear that we only stay in recovery if
 standby_mode is directly turned on.  This makes the whole check for a
 specially named file unnecessary, IMO: we should just check the value of
 standby_mode (which is off by default).

 So, what happens when someone does pg_ctl promote?  Somehow
 standby_mode needs to get set to off.  Maybe we write standby_mode =
 off to postgresql.auto.conf?
 
 Uhh, and then not work for the sane folks who disable
 postgresql.auto.conf?  No thanks..

Other ideas then, without reverting to the old system?  Being able to
promote servers over port 5432 will be a huge advantage for
container-based systems, so I don't want to give that up as a feature.

 HOWEVER, there's a clear out for this with conf.d.  If we enable conf.d
 by default, then we can simply put recovery settings in conf.d, and
 since that specific file (which can have whatever name the user chooses)
 will not be part of backups, it would have the same advantage as
 recovery.conf, without the drawbacks.
 
 conf.d is a possibility, but there may be environments where the
 postgres users doesn't have access to write into the conf.d directrory..
 Not sure if that'd be an issue for what you're suggesting but wanted to
 point it out.

It's not a real issue for any use-case I'm currently dealing with.
Would this approach break things for other people?

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


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


Re: [HACKERS] Turning recovery.conf into GUCs

2014-11-21 Thread Amit Kapila
On Sat, Nov 22, 2014 at 12:24 AM, Stephen Frost sfr...@snowman.net wrote:

 * Josh Berkus (j...@agliodbs.com) wrote:
   Either way, from the code it is clear that we only stay in recovery if
   standby_mode is directly turned on.  This makes the whole check for a
   specially named file unnecessary, IMO: we should just check the value
of
   standby_mode (which is off by default).
 
  So, what happens when someone does pg_ctl promote?  Somehow
  standby_mode needs to get set to off.  Maybe we write standby_mode =
  off to postgresql.auto.conf?

 Uhh, and then not work for the sane folks who disable
 postgresql.auto.conf?  No thanks..


What exactly you mean by 'disable postgresql.auto.conf',  do you
mean user runs Alter System to remove that entry or manually disable
some particular entry?

By default postgresql.auto.conf is always read, so if there is any
setting present in that file, that will taken into consideration.


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


Re: [HACKERS] Turning recovery.conf into GUCs

2014-06-04 Thread Josh Berkus
All,

Can we get this patch going again for 9.5?


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


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


Re: [HACKERS] Turning recovery.conf into GUCs

2014-06-04 Thread Andres Freund
Hi,

On 2014-06-04 16:32:33 -0700, Josh Berkus wrote:
 Can we get this patch going again for 9.5?

A patch gets going by somebody working on it.

Greetings,

Andres Freund

-- 
 Andres Freund 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


Re: [HACKERS] Turning recovery.conf into GUCs

2014-06-04 Thread Joshua D. Drake


On 06/04/2014 04:35 PM, Andres Freund wrote:


Hi,

On 2014-06-04 16:32:33 -0700, Josh Berkus wrote:

Can we get this patch going again for 9.5?


A patch gets going by somebody working on it.


Well yes, but it is also great to have someone remind others that it is 
of interest.


JD




Greetings,

Andres Freund




--
Command Prompt, Inc. - http://www.commandprompt.com/  509-416-6579
PostgreSQL Support, Training, Professional Services and Development
High Availability, Oracle Conversion, Postgres-XC, @cmdpromptinc
Political Correctness is for cowards.


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


Re: [HACKERS] Turning recovery.conf into GUCs

2014-02-18 Thread Michael Paquier
This patch is in Waiting for Author for a couple of weeks and has
received a review at least from Andres during this commit fest. As the
situation is not much progressing, I am going to mark it as Returned
with feedback.
If there are any problems with that please let me know.
Thanks,
-- 
Michael


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


Re: [HACKERS] Turning recovery.conf into GUCs

2014-01-23 Thread Andres Freund
Hi,

On 2014-01-15 02:00:51 -0500, Jaime Casanova wrote:
 On Mon, Nov 18, 2013 at 12:27 PM, Andres Freund and...@2ndquadrant.com 
 wrote:
  Hi,
 
  On 2013-11-15 22:38:05 -0500, Jaime Casanova wrote:
  sorry, i clearly misunderstood you. attached a rebased patch with
  those functions removed.
 
  * --write-standby-enable seems to loose quite some functionality in
comparison to --write-recovery-conf since it doesn't seem to set
primary_conninfo, standby anymore.
 
 we can add code that looks for postgresql.conf in $PGDATA but if
 postgresql.conf is not there (like the case in debian, there is not
 much we can do about it) or we can write a file ready to be included
 in postgresql.conf, any sugestion?

People might not like me for the suggestion, but I think we should
simply always include a 'recovery.conf' in $PGDATA
unconditionally. That'd make this easier.
Alternatively we could pass a filename to --write-recovery-conf.

  * CheckRecoveryReadyFile() doesn't seem to be a very descriptive
function name.
 
 I left it as CheckStartingAsStandby() but i still have a problem of
 this not being completely clear. this function is useful for standby
 or pitr.
 which leads me to the other problem i have: the recovery trigger file,
 i have left it as standby.enabled but i still don't like it.

 recovery.trigger (Andres objected on this name)
 forced_recovery.trigger
 user_forced_recovery.trigger

stay_in_recovery.trigger? That'd be pretty clear for anybody involved in
pg, but otherwise...


  * the description of archive_cleanup_command seems wrong to me.
 
 why? it seems to be the same that was in recovery.conf. where did you
 see the description you're complaining at?

I dislike the description in guc.c

 + {archive_cleanup_command, PGC_POSTMASTER, 
 WAL_ARCHIVE_RECOVERY,
 + gettext_noop(Sets the shell command that will be 
 executed at every restartpoint.),
 + NULL
 + },
 + archive_cleanup_command,

previously it was:

 -# specifies an optional shell command to execute at every restartpoint.
 -# This can be useful for cleaning up the archive of a standby server.

  * Why the PG_TRY/PG_CATCH in check_recovery_target_time? Besides being
really strangely formatted (multiline :? inside a function?) it
doesn't a) seem to be correct to ignore potential memory allocation
errors by just switching back into the context that just errored out,
and continue to work there b) forgo cleanup by just continuing as if
nothing happened. That's unlikely to be acceptable.
 
 the code that read recovery.conf didn't has that, so i just removed it

Well, that's not necessarily correct. recovery.conf was only read during
startup, while this is read on SIGHUP.

  * Why do you include xlog_internal.h in guc.c and not xlog.h?

 we actually need both but including xlog_internal.h also includes xlog.h
 i added xlog.h and if someone things is enough only putting
 xlog_internal.h let me know

What's required from xlog_internal.h?

 diff --git a/src/backend/access/transam/xlog.c 
 b/src/backend/access/transam/xlog.c
 index b53ae87..54f6a0d 100644
 --- a/src/backend/access/transam/xlog.c
 +++ b/src/backend/access/transam/xlog.c
 @@ -64,11 +64,12 @@
  extern uint32 bootstrap_data_checksum_version;
  
  /* File path names (all relative to $PGDATA) */
 -#define RECOVERY_COMMAND_FILErecovery.conf
 -#define RECOVERY_COMMAND_DONErecovery.done
 +#define RECOVERY_ENABLE_FILE standby.enabled

Imo the file and variable names should stay coherent.

 +/* recovery.conf is not supported anymore */
 +#define RECOVERY_COMMAND_FILErecovery.conf

 +bool StandbyModeRequested = false;
 +static TimestampTz recoveryDelayUntilTime;

This imo should be lowercase now, the majority of GUC variables are.

 +/* are we currently in standby mode? */
 +bool StandbyMode = false;

Why did you move this?

 - if (rtliGiven)
 + if (strcmp(recovery_target_timeline_string, ) != 0)
   {

Why not have the convention that NULL indicates a unset target_timeline
like you use for other GUCs? Mixing things like this is confusing.

Why is recovery_target_timeline stored as a string? Because it's a
unsigned int? If so, you should have an assign hook setting up a)
rtliGiven, b) properly typed variable.

 - if (rtli)
 + if (recovery_target_timeline)
   {
   /* Timeline 1 does not have a history file, all else 
 should */
 - if (rtli != 1  !existsTimeLineHistory(rtli))
 + if (recovery_target_timeline != 1  
 + 
 !existsTimeLineHistory(recovery_target_timeline))
   ereport(FATAL,
   (errmsg(recovery target 
 timeline %u does not exist,
 - rtli)));
 - recoveryTargetTLI = rtli;

Re: [HACKERS] Turning recovery.conf into GUCs

2014-01-15 Thread Robert Haas
On Wed, Jan 15, 2014 at 2:06 AM, Jaime Casanova ja...@2ndquadrant.com wrote:
 On Wed, Jan 15, 2014 at 2:00 AM, Jaime Casanova ja...@2ndquadrant.com wrote:
 On Mon, Nov 18, 2013 at 12:27 PM, Andres Freund and...@2ndquadrant.com 
 wrote:

 * Maybe we should rename names like pause_at_recovery_target to
   recovery_pause_at_target? Since we already force everyone to bother
   changing their setup...

 i don't have a problem with this, anyone else? if no one speaks i will
 do what Andres suggests


 Actually Michael had objected to this idea but i forgot about that...
 so i will wait for some consensus (my personal opinion is that
 Michael's argument is a good one)

I prefer the current name.  It's more like the way English is spoken.

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


Re: [HACKERS] Turning recovery.conf into GUCs

2014-01-14 Thread Jaime Casanova
On Wed, Jan 15, 2014 at 2:00 AM, Jaime Casanova ja...@2ndquadrant.com wrote:
 On Mon, Nov 18, 2013 at 12:27 PM, Andres Freund and...@2ndquadrant.com 
 wrote:

 * Maybe we should rename names like pause_at_recovery_target to
   recovery_pause_at_target? Since we already force everyone to bother
   changing their setup...

 i don't have a problem with this, anyone else? if no one speaks i will
 do what Andres suggests


Actually Michael had objected to this idea but i forgot about that...
so i will wait for some consensus (my personal opinion is that
Michael's argument is a good one)

-- 
Jaime Casanova www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación
Phone: +593 4 5107566 Cell: +593 987171157


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


Re: [HACKERS] Turning recovery.conf into GUCs

2013-11-20 Thread Jaime Casanova
On Tue, Nov 19, 2013 at 3:32 PM, Andres Freund and...@2ndquadrant.com wrote:
 On 2013-11-19 22:09:48 +0900, Michael Paquier wrote:
 On Tue, Nov 19, 2013 at 2:27 AM, Andres Freund and...@2ndquadrant.com 
 wrote:

  * I am not sure I like recovery.trigger as a name. It seems to close
to what I've seen people use to trigger failover and too close to
trigger_file.

 This name was chosen and kept in accordance to the spec of this
 feature. Looks fine for me...

 I still think start_as_standby.trigger or such would be much clearer
 and far less likely to be confused with the promotion trigger file.


the function of the file is to inform the server it's in recovery and
it needs to consider recovery parameters, not to make the server a
standby. yes, i admit that is half the way to make the server a
standby. for example, if you are doing PITR and stopping the server
before some specific point (recovery_target_*) then
start_as_standby.trigger will has no meaning and could confuse
people


  * You made recovery_target_inclusive/pause_at_recovery_target PGC_SIGHUP
- did you review that actually works? Imo that should be changed in a
separate commit.

 Yep, I'd rather see those parameters as PGC_POSTMASTER... As of now,
 once recovery is started those parameter values do not change once
 readRecoveryCommandFile is kicked. Having them SIGHUP would mean that
 you could change them during recovery. Sounds kind of dangerous, no?

 I think it's desirable to make them changeable during recovery, but it's
 a separate patch.


uh! i read the patch and miss that. will change


  * Why did you change some of the recovery gucs to lowercase names, but
left out XLogRestoreCommand?

 This was part of the former patch, perhaps you are right and keeping
 the names as close as possible to the old ones would make sense to
 facilitate maintenance across versions.

 I think lowercase is slightly more consistent with the majority of the
 other GUCs, but if you change it you should change all the new GUC variables.


This one was my change, in the original patch is called
restore_command and i changed it because archive_command's parameter
is called XLogArchiveCommand so i wanted the name to follow the same
format.

i can return it to the original name if no one likes XLogRestoreCommand

-- 
Jaime Casanova www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación
Phone: +593 4 5107566 Cell: +593 987171157


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


Re: [HACKERS] Turning recovery.conf into GUCs

2013-11-20 Thread Andres Freund
On 2013-11-19 22:24:19 -0500, Tom Lane wrote:
 Andres Freund and...@2ndquadrant.com writes:
  On 2013-11-19 22:09:48 +0900, Michael Paquier wrote:
  On Tue, Nov 19, 2013 at 2:27 AM, Andres Freund and...@2ndquadrant.com 
  wrote:
  * Why did you change some of the recovery gucs to lowercase names, but
  left out XLogRestoreCommand?
 
  This was part of the former patch, perhaps you are right and keeping
  the names as close as possible to the old ones would make sense to
  facilitate maintenance across versions.
 
  I think lowercase is slightly more consistent with the majority of the
  other GUCs, but if you change it you should change all the new GUC 
  variables.
 
 Please *don't* create any more mixed-case GUC names.  The spelling of
 TimeZone and the one or two other historical exceptions was a very
 unfortunate thing; it's confused more people than it's helped.
 Put in some underscores if you feel a need for word boundaries.

That's a misunderstanding - I was only talking about the variables below
the GUCs, no the GUC's name. The patch changed quite some variable
names, around, but left others leaving an inconsistent casing of related
variables...

Greetings,

Andres Freund

-- 
 Andres Freund 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


Re: [HACKERS] Turning recovery.conf into GUCs

2013-11-20 Thread Andres Freund
On 2013-11-20 08:10:44 -0500, Jaime Casanova wrote:
 On Tue, Nov 19, 2013 at 3:32 PM, Andres Freund and...@2ndquadrant.com wrote:
  On 2013-11-19 22:09:48 +0900, Michael Paquier wrote:
  On Tue, Nov 19, 2013 at 2:27 AM, Andres Freund and...@2ndquadrant.com 
  wrote:
 
   * I am not sure I like recovery.trigger as a name. It seems to close
 to what I've seen people use to trigger failover and too close to
 trigger_file.
 
  This name was chosen and kept in accordance to the spec of this
  feature. Looks fine for me...
 
  I still think start_as_standby.trigger or such would be much clearer
  and far less likely to be confused with the promotion trigger file.
 
 
 the function of the file is to inform the server it's in recovery and
 it needs to consider recovery parameters, not to make the server a
 standby. yes, i admit that is half the way to make the server a
 standby. for example, if you are doing PITR and stopping the server
 before some specific point (recovery_target_*) then
 start_as_standby.trigger will has no meaning and could confuse
 people

'recovery' includes crash recovery, that's why I quite dislike your
function name since it's not crash recovery you're checking for since
during that we certainly do not want to interpet those parameters.

Greetings,

Andres Freund

-- 
 Andres Freund 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


Re: [HACKERS] Turning recovery.conf into GUCs

2013-11-19 Thread Michael Paquier
On Tue, Nov 19, 2013 at 2:27 AM, Andres Freund and...@2ndquadrant.com wrote:
 * --write-standby-enable seems to loose quite some functionality in
   comparison to --write-recovery-conf since it doesn't seem to set
   primary_conninfo, standby anymore.
Yes... The idea here might be to generate a new file that is then
included in postgresql.conf or to add parameters at the bottom of
postgresql.conf itself. The code for plain base backup is
straight-forward, but it could get ugly for the tar part...

 * CheckRecoveryReadyFile() doesn't seem to be a very descriptive
   function name.
CheckRecoveryTriggerPresence?

 * Why does StartupXLOG() now use ArchiveRecoveryRequested 
   StandbyModeRequested instead of just the former?
 * I am not sure I like recovery.trigger as a name. It seems to close
   to what I've seen people use to trigger failover and too close to
   trigger_file.
This name was chosen and kept in accordance to the spec of this
feature. Looks fine for me...

 * You made recovery_target_inclusive/pause_at_recovery_target PGC_SIGHUP
   - did you review that actually works? Imo that should be changed in a
   separate commit.

Yep, I'd rather see those parameters as PGC_POSTMASTER... As of now,
once recovery is started those parameter values do not change once
readRecoveryCommandFile is kicked. Having them SIGHUP would mean that
you could change them during recovery. Sounds kind of dangerous, no?

 * Maybe we should rename names like pause_at_recovery_target to
   recovery_pause_at_target? Since we already force everyone to bother
   changing their setup...

I disagree here. It is true that this patch introduces many changes
with a new configuration file layer, but this idea with this patch was
to allow people to reuse their old recovery.conf as it is. And what is
actually wrong with pause_at_recovery_target?


 * the description of archive_cleanup_command seems wrong to me.
 * Why did you change some of the recovery gucs to lowercase names, but
   left out XLogRestoreCommand?

This was part of the former patch, perhaps you are right and keeping
the names as close as possible to the old ones would make sense to
facilitate maintenance across versions.


 * Why the PG_TRY/PG_CATCH in check_recovery_target_time? Besides being
   really strangely formatted (multiline :? inside a function?) it
   doesn't a) seem to be correct to ignore potential memory allocation
   errors by just switching back into the context that just errored out,
   and continue to work there b) forgo cleanup by just continuing as if
   nothing happened. That's unlikely to be acceptable.

Interestingly, that was part of the first versions of the patch as
well. I don't recall modifying anything in this area when I hacked
that... But yes it should be modified to something like what is in
check_recovery_target_xid.

Regards,
-- 
Michael


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


Re: [HACKERS] Turning recovery.conf into GUCs

2013-11-19 Thread Andres Freund
On 2013-11-19 22:09:48 +0900, Michael Paquier wrote:
 On Tue, Nov 19, 2013 at 2:27 AM, Andres Freund and...@2ndquadrant.com wrote:
  * --write-standby-enable seems to loose quite some functionality in
comparison to --write-recovery-conf since it doesn't seem to set
primary_conninfo, standby anymore.

 Yes... The idea here might be to generate a new file that is then
 included in postgresql.conf or to add parameters at the bottom of
 postgresql.conf itself. The code for plain base backup is
 straight-forward, but it could get ugly for the tar part...

Well, just removing most of the feature - which the current patch seems
to be doing - looks like a regression to me, so it has to be either
fixed or explicitly discussed.

  * CheckRecoveryReadyFile() doesn't seem to be a very descriptive
function name.

 CheckRecoveryTriggerPresence?

CheckStartingAsStandby()? Recovery alone doesn't say very much.

  * Why does StartupXLOG() now use ArchiveRecoveryRequested 
StandbyModeRequested instead of just the former?

?

  * I am not sure I like recovery.trigger as a name. It seems to close
to what I've seen people use to trigger failover and too close to
trigger_file.

 This name was chosen and kept in accordance to the spec of this
 feature. Looks fine for me...

I still think start_as_standby.trigger or such would be much clearer
and far less likely to be confused with the promotion trigger file.

  * You made recovery_target_inclusive/pause_at_recovery_target PGC_SIGHUP
- did you review that actually works? Imo that should be changed in a
separate commit.
 
 Yep, I'd rather see those parameters as PGC_POSTMASTER... As of now,
 once recovery is started those parameter values do not change once
 readRecoveryCommandFile is kicked. Having them SIGHUP would mean that
 you could change them during recovery. Sounds kind of dangerous, no?

I think it's desirable to make them changeable during recovery, but it's
a separate patch.

  * Maybe we should rename names like pause_at_recovery_target to
recovery_pause_at_target? Since we already force everyone to bother
changing their setup...

 I disagree here. It is true that this patch introduces many changes
 with a new configuration file layer, but this idea with this patch was
 to allow people to reuse their old recovery.conf as it is. And what is
 actually wrong with pause_at_recovery_target?

That nearly all the other variables start with recovery_. But I don't
feel very strongly abou thtis.

  * Why did you change some of the recovery gucs to lowercase names, but
left out XLogRestoreCommand?
 
 This was part of the former patch, perhaps you are right and keeping
 the names as close as possible to the old ones would make sense to
 facilitate maintenance across versions.

I think lowercase is slightly more consistent with the majority of the
other GUCs, but if you change it you should change all the new GUC variables.

Greetings,

Andres Freund

-- 
 Andres Freund 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


Re: [HACKERS] Turning recovery.conf into GUCs

2013-11-19 Thread Tom Lane
Andres Freund and...@2ndquadrant.com writes:
 On 2013-11-19 22:09:48 +0900, Michael Paquier wrote:
 On Tue, Nov 19, 2013 at 2:27 AM, Andres Freund and...@2ndquadrant.com 
 wrote:
 * Why did you change some of the recovery gucs to lowercase names, but
 left out XLogRestoreCommand?

 This was part of the former patch, perhaps you are right and keeping
 the names as close as possible to the old ones would make sense to
 facilitate maintenance across versions.

 I think lowercase is slightly more consistent with the majority of the
 other GUCs, but if you change it you should change all the new GUC variables.

Please *don't* create any more mixed-case GUC names.  The spelling of
TimeZone and the one or two other historical exceptions was a very
unfortunate thing; it's confused more people than it's helped.
Put in some underscores if you feel a need for word boundaries.

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


Re: [HACKERS] Turning recovery.conf into GUCs

2013-11-18 Thread Andres Freund
Hi,

On 2013-11-15 22:38:05 -0500, Jaime Casanova wrote:
 sorry, i clearly misunderstood you. attached a rebased patch with
 those functions removed.

* --write-standby-enable seems to loose quite some functionality in
  comparison to --write-recovery-conf since it doesn't seem to set
  primary_conninfo, standby anymore.
* CheckRecoveryReadyFile() doesn't seem to be a very descriptive
  function name.
* Why does StartupXLOG() now use ArchiveRecoveryRequested 
  StandbyModeRequested instead of just the former?
* I am not sure I like recovery.trigger as a name. It seems to close
  to what I've seen people use to trigger failover and too close to
  trigger_file.
* You check for a trigger file in a way that's not compatible with it
  being NULL. Why did you change that?
-   if (TriggerFile == NULL)
+   if (!trigger_file[0])
* You made recovery_target_inclusive/pause_at_recovery_target PGC_SIGHUP
  - did you review that actually works? Imo that should be changed in a
  separate commit.
* Maybe we should rename names like pause_at_recovery_target to
  recovery_pause_at_target? Since we already force everyone to bother
  changing their setup...
* the description of archive_cleanup_command seems wrong to me.
* Why did you change some of the recovery gucs to lowercase names, but
  left out XLogRestoreCommand?
* Why the PG_TRY/PG_CATCH in check_recovery_target_time? Besides being
  really strangely formatted (multiline :? inside a function?) it
  doesn't a) seem to be correct to ignore potential memory allocation
  errors by just switching back into the context that just errored out,
  and continue to work there b) forgo cleanup by just continuing as if
  nothing happened. That's unlikely to be acceptable.
* You access recovery_target_name[0] unconditionally, although it's
  intialized to NULL.
* Generally, ISTM there's too much logic in the assign hooks.
* Why do you include xlog_internal.h in guc.c and not xlog.h?

Ok, I think that's enough for now ;)

Greetings,

Andres Freund

-- 
 Andres Freund 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


Re: [HACKERS] Turning recovery.conf into GUCs

2013-11-15 Thread Jaime Casanova
On Fri, Nov 15, 2013 at 9:28 AM, Peter Eisentraut pete...@gmx.net wrote:
 On 11/13/13, 12:17 AM, Jaime Casanova wrote:
 I have rebased Michael Paquier's patch and did a few changes:

 * changed standby.enabled filename to recovery.trigger
 * make archive_command a SIGHUP parameter again
 * make restore_command a SIGHUP parameter
 * rename restore_command variable to XLogRestoreCommand to match
 XLogArchiveCommand

 Please check for compiler warnings in pg_basebackup:

 pg_basebackup.c:1109:1: warning: ‘escapeConnectionParameter’ defined but not 
 used [-Wunused-function]
 pg_basebackup.c:1168:1: warning: ‘escape_quotes’ defined but not used 
 [-Wunused-function]


those are functions that are no longer used but Josh considered they
could become useful before release.
i can put them inside #ifdef _NOT_USED_ decorations or just remove
them now and if/when we find some use for them re add them

-- 
Jaime Casanova www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación
Phone: +593 4 5107566 Cell: +593 987171157


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


Re: [HACKERS] Turning recovery.conf into GUCs

2013-11-15 Thread Peter Eisentraut
On 11/13/13, 12:17 AM, Jaime Casanova wrote:
 I have rebased Michael Paquier's patch and did a few changes:
 
 * changed standby.enabled filename to recovery.trigger
 * make archive_command a SIGHUP parameter again
 * make restore_command a SIGHUP parameter
 * rename restore_command variable to XLogRestoreCommand to match
 XLogArchiveCommand

Please check for compiler warnings in pg_basebackup:

pg_basebackup.c:1109:1: warning: ‘escapeConnectionParameter’ defined but not 
used [-Wunused-function]
pg_basebackup.c:1168:1: warning: ‘escape_quotes’ defined but not used 
[-Wunused-function]



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


Re: [HACKERS] Turning recovery.conf into GUCs

2013-11-15 Thread Michael Paquier
On Fri, Nov 15, 2013 at 11:38 PM, Jaime Casanova ja...@2ndquadrant.com wrote:
 those are functions that are no longer used but Josh considered they
 could become useful before release.
 i can put them inside #ifdef _NOT_USED_ decorations or just remove
 them now and if/when we find some use for them re add them
Why not simply removing them? They will be kept in the git history either way.
-- 
Michael


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


Re: [HACKERS] Turning recovery.conf into GUCs

2013-11-15 Thread Josh Berkus
On 11/15/2013 06:38 AM, Jaime Casanova wrote:
  Please check for compiler warnings in pg_basebackup:
 
  pg_basebackup.c:1109:1: warning: ‘escapeConnectionParameter’ defined but 
  not used [-Wunused-function]
  pg_basebackup.c:1168:1: warning: ‘escape_quotes’ defined but not used 
  [-Wunused-function]
 
 those are functions that are no longer used but Josh considered they
 could become useful before release.
 i can put them inside #ifdef _NOT_USED_ decorations or just remove
 them now and if/when we find some use for them re add them

Wait, which Josh?  Not me ...

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


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


Re: [HACKERS] Turning recovery.conf into GUCs

2013-10-21 Thread Josh Berkus
On 10/18/2013 02:36 PM, Andres Freund wrote:
  What will likely change first is Slony and Bucardo, who have a strong
  interest in dumping triggers and queues.
 But I don't understand what that has to do with recovery.conf and
 breakage around it.

The simple thinking is this: if we announce and promote new replication,
then our users who do upgrade are going to expect to upgrade their
replication tools at the same time, even if they're not using the new
replication.  That is people will look for a repmgr 2.0 / OmniPITR 1.5
and update to it.

Now, as a tool author, I know that supporting both models is going to be
annoying.  But necessary.

And, as I said before, we need to do the GUC merger in the same release
we introduce configuration directory (or after it).

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


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


Re: [HACKERS] Turning recovery.conf into GUCs

2013-10-21 Thread Jaime Casanova
On Mon, Oct 21, 2013 at 11:53 AM, Josh Berkus j...@agliodbs.com wrote:
 On 10/18/2013 02:36 PM, Andres Freund wrote:
  What will likely change first is Slony and Bucardo, who have a strong
  interest in dumping triggers and queues.
 But I don't understand what that has to do with recovery.conf and
 breakage around it.

 The simple thinking is this: if we announce and promote new replication,
 then our users who do upgrade are going to expect to upgrade their
 replication tools at the same time, even if they're not using the new
 replication.  That is people will look for a repmgr 2.0 / OmniPITR 1.5
 and update to it.

 Now, as a tool author, I know that supporting both models is going to be
 annoying.  But necessary.


AFAIU, even if we get in all about logical replication today that
won't affect tools that manage binary replication.

 And, as I said before, we need to do the GUC merger in the same release
 we introduce configuration directory (or after it).


you mean the ALTER SYSTEM syntax? anyway, why that restriction?

-- 
Jaime Casanova www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación
Phone: +593 4 5107566 Cell: +593 987171157


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


Re: [HACKERS] Turning recovery.conf into GUCs

2013-10-21 Thread Josh Berkus
Jaime,

 And, as I said before, we need to do the GUC merger in the same release
 we introduce configuration directory (or after it).

 
 you mean the ALTER SYSTEM syntax? anyway, why that restriction?

No, I'm referring to the proposal to have an automatically created and
included conf.d directory for extra configuration files.

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


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


Re: [HACKERS] Turning recovery.conf into GUCs

2013-10-21 Thread Jaime Casanova
On Mon, Oct 21, 2013 at 2:57 PM, Josh Berkus j...@agliodbs.com wrote:
 Jaime,

 And, as I said before, we need to do the GUC merger in the same release
 we introduce configuration directory (or after it).


 you mean the ALTER SYSTEM syntax? anyway, why that restriction?

 No, I'm referring to the proposal to have an automatically created and
 included conf.d directory for extra configuration files.


9.3 has include_dir and postgresql.conf has this setted:

#include_dir = 'conf.d' # include files ending in '.conf' from
# directory 'conf.d'


anything before this should be up to the packagers, no?

or, do you mean something else?

-- 
Jaime Casanova www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación
Phone: +593 4 5107566 Cell: +593 987171157


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


Re: [HACKERS] Turning recovery.conf into GUCs

2013-10-21 Thread Josh Berkus

 9.3 has include_dir and postgresql.conf has this setted:
 
 #include_dir = 'conf.d' # include files ending in '.conf' from
 # directory 'conf.d'
 
 
 anything before this should be up to the packagers, no?
 
 or, do you mean something else?

Well, I was just pointing out that it would make things *much* easier
for tool authors if conf.d were enabled by default, instead of disabled
by default.  Then they could change tools to write a recovery.conf
file to conf.d.

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


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


Re: [HACKERS] Turning recovery.conf into GUCs

2013-10-18 Thread Michael Paquier
On Fri, Oct 18, 2013 at 1:13 PM, Jaime Casanova ja...@2ndquadrant.com wrote:
 = Code  functionality =

 +   {restore_command, PGC_POSTMASTER, WAL_ARCHIVE_RECOVERY,
 +   {archive_cleanup_command, PGC_POSTMASTER, WAL_ARCHIVE_RECOVERY,
 +   {recovery_end_command, PGC_POSTMASTER, WAL_ARCHIVE_RECOVERY,
 +   {recovery_target_xid, PGC_POSTMASTER, WAL_RECOVERY_TARGET,
 +   {recovery_target_name, PGC_POSTMASTER, WAL_RECOVERY_TARGET,
 +   {recovery_target_time, PGC_POSTMASTER, WAL_RECOVERY_TARGET,
 +   {trigger_file, PGC_POSTMASTER, REPLICATION_STANDBY,

 Not sure about these ones

 +   {recovery_target_timeline, PGC_POSTMASTER, WAL_RECOVERY_TARGET,
 +   {primary_conninfo, PGC_POSTMASTER, REPLICATION_STANDBY,

 It would be really nice to change these on the fly; it would help a lot
 of issues with minor changes to replication config.  I can understand,
 though, that the replication code might not be prepared for that.


 well, archive_command can be changed right now with a SIGHUP so at
 least that one shouldn't change... and i don't think most of these are
 too different. even if we are not sure we can do this now and change
 them as SIGHUP later
Changing those parameters don't really matter as long as the node is
not performing a recovery IMO, but I'd rather see a careful approach
here and let all those parameters as PGC_POSTMASTER for now to avoid
any surprises. Perhaps a second patch on top of this one could be the
addition of context name like SIGHUP_RECOVERY, aka just allow those
parameters to be updated with SIGHUP as long as the node is not in
recovery.
-- 
Michael


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


Re: [HACKERS] Turning recovery.conf into GUCs

2013-10-18 Thread Josh Berkus
Jaime,

 Except that we'll want 9.4's -R to do something, probably create a file
 called conf.d/replication.conf.  Mind you, it won't need the same wonky
 quoting stuff.

 
 Currently the patch uses -R to create the recovery trigger file

Right, I'm saying that we'll want to do better than that for release,
but that's dependant on committing the conf directory patch.

Note that this change makes committing the conf.d patch extra-important;
it's going to be a LOT easier to upgrade tools for 9.4 if we have that.

 well, after upgrade you should do checks. and even if it happens,
 after it happens once people will be aware of the change.
 now, some suggestions were made to avoid the problem. 1) read the file
 if exists last in the process of postgresql.conf, 2) add a GUC
 indicating if it should read it and include it (not using it as a
 trigger file). another one, 3) include in this release an
 include_if_exists directive and give a warning if it sees the file
 then include it, on next release remove the include_if_exists (at
 least that way people will be warned before breaking compatibility)

I think all of these suggestions just make our code more complicated
without improving the upgrade situation.

The reason given (and I think it's pretty good) for erroring on
recovery.conf is that we don't want people to accidentally take a server
out of replication because they didn't check which version of PostgreSQL
they are on.

 *on the other hand*, if we prevent creation of a configuration file
 named recovery.conf, then we block efforts to write
 backwards-compatible management utilities.

 
 and all tools and procedures that currently exists.

Right.  However, exploring your suggestions above, none of those
workarounds prevent breakage.  And in some cases, they make the breakage
less obvious than the current patch does.  If repmgr 1.2 / OmniPITR 1.2
won't work correctly with 9.4, then we want those tools to break at
upgrade time, not later when the user is trying to fail over.

For that matter, 9.4 is a very good time (relatively speaking) to break
replication tools because the new logical replication is going to cause
everyone to rev their tools anyway.

This kind of breakage alone might end up being a good reason to call the
next version 10.0.

 well, there should be good solutions... maybe we haven't thought them yet.
 anyway, we can't postpone the decision forever. we need to make a
 decision and stick with it otherwise this patch will be stalled N
 releases for no good reason

I think if there were a good solution, sometime in the last 1.5 years
someone would have suggested it.  Gods know Simon has tried.

 exactly as it is now, if it sees the recovery trigger file, then it
 starts ArchiveRecovery and after it finish delete the file (the only
 difference) and increment the timeline

OK, so if I'm doing a PITR recovery, I just put the recovery variables
into postgresql.conf, then?

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


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


Re: [HACKERS] Turning recovery.conf into GUCs

2013-10-18 Thread Jaime Casanova
On Fri, Oct 18, 2013 at 11:32 AM, Josh Berkus j...@agliodbs.com wrote:

 exactly as it is now, if it sees the recovery trigger file, then it
 starts ArchiveRecovery and after it finish delete the file (the only
 difference) and increment the timeline

 OK, so if I'm doing a PITR recovery, I just put the recovery variables
 into postgresql.conf, then?


create a recovery trigger file (called standby.enabled in current
patch) in $PGDATA and start the server

-- 
Jaime Casanova www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación
Phone: +593 4 5107566 Cell: +593 987171157


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


Re: [HACKERS] Turning recovery.conf into GUCs

2013-10-18 Thread Andres Freund
On 2013-10-18 09:32:15 -0700, Josh Berkus wrote:
 For that matter, 9.4 is a very good time (relatively speaking) to break
 replication tools because the new logical replication is going to cause
 everyone to rev their tools anyway.

We're hopefully getting changeset extraction in, but there's little
chance of a full blown replication solution making it in in 9.4...

Greetings,

Andres Freund

-- 
 Andres Freund 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


Re: [HACKERS] Turning recovery.conf into GUCs

2013-10-18 Thread Josh Berkus
On 10/18/2013 12:29 PM, Andres Freund wrote:
 On 2013-10-18 09:32:15 -0700, Josh Berkus wrote:
 For that matter, 9.4 is a very good time (relatively speaking) to break
 replication tools because the new logical replication is going to cause
 everyone to rev their tools anyway.
 
 We're hopefully getting changeset extraction in, but there's little
 chance of a full blown replication solution making it in in 9.4...

I thought changeset extraction was the only thing going into core?  What
else do we need?

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


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


Re: [HACKERS] Turning recovery.conf into GUCs

2013-10-18 Thread Andres Freund
On 2013-10-18 13:16:52 -0700, Josh Berkus wrote:
 On 10/18/2013 12:29 PM, Andres Freund wrote:
  On 2013-10-18 09:32:15 -0700, Josh Berkus wrote:
  For that matter, 9.4 is a very good time (relatively speaking) to break
  replication tools because the new logical replication is going to cause
  everyone to rev their tools anyway.
  
  We're hopefully getting changeset extraction in, but there's little
  chance of a full blown replication solution making it in in 9.4...
 
 I thought changeset extraction was the only thing going into core?  What
 else do we need?

Well, I personally want more in core mid/long term, but anyway.

Without released, proven and stable logical in-core replication
technology using this, I don't see why repmgr or something related would
need/want to change?

Greetings,

Andres Freund

-- 
 Andres Freund 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


Re: [HACKERS] Turning recovery.conf into GUCs

2013-10-18 Thread Josh Berkus
On 10/18/2013 01:35 PM, Andres Freund wrote:
 On 2013-10-18 13:16:52 -0700, Josh Berkus wrote:
 I thought changeset extraction was the only thing going into core?  What
 else do we need?
 
 Well, I personally want more in core mid/long term, but anyway.

I've lost track of the plan, then.

Hmmm ... we need replication of DDL commands, no?

 Without released, proven and stable logical in-core replication
 technology using this, I don't see why repmgr or something related would
 need/want to change?

Repmgr is designed to manage binary replication, not perform it.

What will likely change first is Slony and Bucardo, who have a strong
interest in dumping triggers and queues.  A contrib module which did the
simplest implementation -- that is, whole-database M-S replication --
would also be a good idea, especially since it would provide an example
of how to build your own.

But I'd be wary of going beyond that in core, because you very quickly
get into the territory of trying to satisfy multiple exclusive
use-cases.  Let's focus on providing a really good API which enables
people to build their own tools.

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


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


Re: [HACKERS] Turning recovery.conf into GUCs

2013-10-18 Thread Andres Freund
On 2013-10-18 14:16:04 -0700, Josh Berkus wrote:
 On 10/18/2013 01:35 PM, Andres Freund wrote:
  On 2013-10-18 13:16:52 -0700, Josh Berkus wrote:
  I thought changeset extraction was the only thing going into core?  What
  else do we need?
  
  Well, I personally want more in core mid/long term, but anyway.
 
 I've lost track of the plan, then.
 
 Hmmm ... we need replication of DDL commands, no?
 
  Without released, proven and stable logical in-core replication
  technology using this, I don't see why repmgr or something related would
  need/want to change?
 
 Repmgr is designed to manage binary replication, not perform it.

Obviously.

 What will likely change first is Slony and Bucardo, who have a strong
 interest in dumping triggers and queues.

But I don't understand what that has to do with recovery.conf and
breakage around it.

 A contrib module which did the
 simplest implementation -- that is, whole-database M-S replication --
 would also be a good idea, especially since it would provide an example
 of how to build your own.
 
 But I'd be wary of going beyond that in core, because you very quickly
 get into the territory of trying to satisfy multiple exclusive
 use-cases.  Let's focus on providing a really good API which enables
 people to build their own tools.

We'll see. I am certain we'll have many discussions about the bits and
pieces you need to build a great replication solution (of which we imo
don't have any yet).

Greetings,

Andres Freund

-- 
 Andres Freund 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


Re: [HACKERS] Turning recovery.conf into GUCs

2013-10-18 Thread Jaime Casanova
On Fri, Oct 18, 2013 at 11:32 AM, Josh Berkus j...@agliodbs.com wrote:
 Jaime,

 well, after upgrade you should do checks. and even if it happens,
 after it happens once people will be aware of the change.
 now, some suggestions were made to avoid the problem. 1) read the file
 if exists last in the process of postgresql.conf, 2) add a GUC
 indicating if it should read it and include it (not using it as a
 trigger file). another one, 3) include in this release an
 include_if_exists directive and give a warning if it sees the file
 then include it, on next release remove the include_if_exists (at
 least that way people will be warned before breaking compatibility)

 I think all of these suggestions just make our code more complicated
 without improving the upgrade situation.


well #3 just add a line in postgresql.conf (an include_if_exists) and
current patch gives an error in case it finds the file (i'm suggesting
to make it a warning instead).
how does that makes our code more complicated?

 The reason given (and I think it's pretty good) for erroring on
 recovery.conf is that we don't want people to accidentally take a server
 out of replication because they didn't check which version of PostgreSQL
 they are on.


well, people will go out of replication also if they forgot the
recovery trigger file
even if they set the variables in postgresql.conf

it happens two me twice today ;)

-- 
Jaime Casanova www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación
Phone: +593 4 5107566 Cell: +593 987171157


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


Re: [HACKERS] Turning recovery.conf into GUCs

2013-10-18 Thread Josh Berkus
On 10/18/2013 02:58 PM, Jaime Casanova wrote:
 well #3 just add a line in postgresql.conf (an include_if_exists) and
 current patch gives an error in case it finds the file (i'm suggesting
 to make it a warning instead).
 how does that makes our code more complicated?

Well, that's a couple extra lines only, I know.  However, it doesn't
actually  help with the breakage any, since recovery.conf *still* won't
work as a trigger file.

The only thing which would prevent breakage (proposed by Simon, I think)
is having recovery.conf have an include_if_exists, AND have
recovery.conf be an 'alternate' name for replication.trigger.  However,
even this would break, and in IMHO ways which would tend to happen at
failover time rather than upgrade time.

To put it clearly: if we're going to have breakage, I want it to be at
upgrade time, when the database is *already down*, and not at failover
time or some other time when downtime is not planned.

 well, people will go out of replication also if they forgot the
 recovery trigger file
 even if they set the variables in postgresql.conf
 
 it happens two me twice today ;)

Right.  What I'd like to avoid is having folks try to use, for example,
repmgr 1.2 with PostgreSQL 9.4 and have their replication break and them
not notice for a couple hours of operation.  I'd rather have PostgreSQL
9.4 refuse to come up, so that they know *immediately* that something is
wrong.

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


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


Re: [HACKERS] Turning recovery.conf into GUCs

2013-10-17 Thread Jaime Casanova
On Wed, Oct 16, 2013 at 1:34 PM, Josh Berkus j...@agliodbs.com wrote:
 Jaime,
 = Building =

 In pg_basebackup we have now 2 unused functions:
 escapeConnectionParameter and escape_quotes. the only use of these
 functions was when that tool created the recovery.conf file so they
 aren't needed anymore.

 Except that we'll want 9.4's -R to do something, probably create a file
 called conf.d/replication.conf.  Mind you, it won't need the same wonky
 quoting stuff.


Currently the patch uses -R to create the recovery trigger file

 1) the file to trigger recovery is now called standby.enabled. I know
 this is because we use the file to also make the node a standby.
 Now, reality is that the file doesn't make the node a standby but the
 combination of this file (which starts recovery) + standby_mode=on.
 so, i agree with the suggestion of naming the file: recovery.trigger

 2) This patch removes the dual existence of recovery.conf: as a state
 file and as a configuration file

 - the former is accomplished by changing the name of the file that
 triggers recovery (from recovery.conf to standby.enabled)
 - the latter is done by moving all parameters to postgresql.conf and
 *not reading* recovery.conf anymore

 so, after applying this patch postgres don't use recovery.conf for
 anything... except for complaining.
 it's completely fair to say we are no longer using that file and
 ignoring its existence, but why we should care if users want to use a
 file with that name for inclusion in postgresql.conf or where they put
 that hypotetic file?

 So this is a bit of a catch-22.  If we allow the user to use a file
 named recovery.conf in $PGDATA, then the user may be unaware that the
 *meaning* of the file has changed, which can result in unplanned
 promotion of replicas after upgrade.


well, after upgrade you should do checks. and even if it happens,
after it happens once people will be aware of the change.
now, some suggestions were made to avoid the problem. 1) read the file
if exists last in the process of postgresql.conf, 2) add a GUC
indicating if it should read it and include it (not using it as a
trigger file). another one, 3) include in this release an
include_if_exists directive and give a warning if it sees the file
then include it, on next release remove the include_if_exists (at
least that way people will be warned before breaking compatibility)

please, keep in mind none of these suggestions include make the
recovery.conf file act as a trigger for recovery

 *on the other hand*, if we prevent creation of a configuration file
 named recovery.conf, then we block efforts to write
 backwards-compatible management utilities.


and all tools and procedures that currently exists.

 AFAIK, there is no good solution for this, which is why it's taken so
 long for us to resolve the general issue.  Given that, I think it's
 better to go for the breakage and all the warnings.  If a user wants a
 file called recovery.conf, put it in the conf.d directory.


well, there should be good solutions... maybe we haven't thought them yet.
anyway, we can't postpone the decision forever. we need to make a
decision and stick with it otherwise this patch will be stalled N
releases for no good reason

 I don't remember, though: how does this patch handle PITR recovery?


exactly as it is now, if it sees the recovery trigger file, then it
starts ArchiveRecovery and after it finish delete the file (the only
difference) and increment the timeline

 = Code  functionality =

 +   {restore_command, PGC_POSTMASTER, WAL_ARCHIVE_RECOVERY,
 +   {archive_cleanup_command, PGC_POSTMASTER, WAL_ARCHIVE_RECOVERY,
 +   {recovery_end_command, PGC_POSTMASTER, WAL_ARCHIVE_RECOVERY,
 +   {recovery_target_xid, PGC_POSTMASTER, WAL_RECOVERY_TARGET,
 +   {recovery_target_name, PGC_POSTMASTER, WAL_RECOVERY_TARGET,
 +   {recovery_target_time, PGC_POSTMASTER, WAL_RECOVERY_TARGET,
 +   {trigger_file, PGC_POSTMASTER, REPLICATION_STANDBY,

 Not sure about these ones

 +   {recovery_target_timeline, PGC_POSTMASTER, WAL_RECOVERY_TARGET,
 +   {primary_conninfo, PGC_POSTMASTER, REPLICATION_STANDBY,

 It would be really nice to change these on the fly; it would help a lot
 of issues with minor changes to replication config.  I can understand,
 though, that the replication code might not be prepared for that.


well, archive_command can be changed right now with a SIGHUP so at
least that one shouldn't change... and i don't think most of these are
too different. even if we are not sure we can do this now and change
them as SIGHUP later

-- 
Jaime Casanova www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación
Phone: +593 4 5107566 Cell: +593 987171157


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


Re: [HACKERS] Turning recovery.conf into GUCs

2013-10-16 Thread Josh Berkus
Jaime,
 = Building =

 In pg_basebackup we have now 2 unused functions:
 escapeConnectionParameter and escape_quotes. the only use of these
 functions was when that tool created the recovery.conf file so they
 aren't needed anymore.

Except that we'll want 9.4's -R to do something, probably create a file
called conf.d/replication.conf.  Mind you, it won't need the same wonky
quoting stuff.

 1) the file to trigger recovery is now called standby.enabled. I know
 this is because we use the file to also make the node a standby.
 Now, reality is that the file doesn't make the node a standby but the
 combination of this file (which starts recovery) + standby_mode=on.
 so, i agree with the suggestion of naming the file: recovery.trigger
 
 2) This patch removes the dual existence of recovery.conf: as a state
 file and as a configuration file
 
 - the former is accomplished by changing the name of the file that
 triggers recovery (from recovery.conf to standby.enabled)
 - the latter is done by moving all parameters to postgresql.conf and
 *not reading* recovery.conf anymore
 
 so, after applying this patch postgres don't use recovery.conf for
 anything... except for complaining.
 it's completely fair to say we are no longer using that file and
 ignoring its existence, but why we should care if users want to use a
 file with that name for inclusion in postgresql.conf or where they put
 that hypotetic file?

So this is a bit of a catch-22.  If we allow the user to use a file
named recovery.conf in $PGDATA, then the user may be unaware that the
*meaning* of the file has changed, which can result in unplanned
promotion of replicas after upgrade.

*on the other hand*, if we prevent creation of a configuration file
named recovery.conf, then we block efforts to write
backwards-compatible management utilities.

AFAIK, there is no good solution for this, which is why it's taken so
long for us to resolve the general issue.  Given that, I think it's
better to go for the breakage and all the warnings.  If a user wants a
file called recovery.conf, put it in the conf.d directory.

I don't remember, though: how does this patch handle PITR recovery?

 = Code  functionality =

 +   {restore_command, PGC_POSTMASTER, WAL_ARCHIVE_RECOVERY,
 +   {archive_cleanup_command, PGC_POSTMASTER, WAL_ARCHIVE_RECOVERY,
 +   {recovery_end_command, PGC_POSTMASTER, WAL_ARCHIVE_RECOVERY,
 +   {recovery_target_xid, PGC_POSTMASTER, WAL_RECOVERY_TARGET,
 +   {recovery_target_name, PGC_POSTMASTER, WAL_RECOVERY_TARGET,
 +   {recovery_target_time, PGC_POSTMASTER, WAL_RECOVERY_TARGET,
 +   {trigger_file, PGC_POSTMASTER, REPLICATION_STANDBY,
 
 Not sure about these ones
 
 +   {recovery_target_timeline, PGC_POSTMASTER, WAL_RECOVERY_TARGET,
 +   {primary_conninfo, PGC_POSTMASTER, REPLICATION_STANDBY,

It would be really nice to change these on the fly; it would help a lot
of issues with minor changes to replication config.  I can understand,
though, that the replication code might not be prepared for that.



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


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