Re: [HACKERS] Turning recovery.conf into GUCs
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
* 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
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
* 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
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
* 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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
* 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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
[HACKERS] Turning recovery.conf into GUCs
Hi everyone,
Seems that the latest patch in this series is:
http://www.postgresql.org/message-id/CAB7nPqRLWLH1b0YsvqYF-zOFjrv4FRiurQ6yqcP3wjBp=tj...@mail.gmail.com
= Applying =
Reading the threads it seems there is consensus in this happening, so
let's move in that direction.
The patch applies almost cleanly except for a minor change in xlog.c
= 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.
= Functionality =
I have been testing functionality and it looks good so far, i still
need to test a few things but i don't expect anything bad.
I have 2 complaints though:
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?
after this patch, recovery.conf will have no special meaning anymore
so let's the user put it whatever files he wants, wherever he wants.
if we really want to warn the user, use WARNING instead of FATAL
= Code functionality =
why did you changed the context in which we can set archive_command?
please, let it as a SIGHUP parameter
- {archive_command, PGC_SIGHUP, WAL_ARCHIVING,
+ {archive_command, PGC_POSTMASTER, WAL_ARCHIVING,
All parameters moved from recovery.conf has been marked as
PGC_POSTMASTER, but most of them are clearly PGC_SIGHUP candidates
+ {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,
This is the only one i agree, should be PGC_POSTMASTER only
+ {standby_mode, PGC_POSTMASTER, REPLICATION_STANDBY,
--
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
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
