Proposal for changes to recovery.conf API

At 2016-08-31 17:15:59 +0100, simon@2ndquadrant.com wrote:

* Recovery parameters would now be part of the main postgresql.conf
infrastructure
Any parameters set in $DATADIR/recovery.conf will be read after the
main parameter file, similar to the way that postgresql.conf.auto is
read.
(Abhijit)

* Parameters
All of the parameters formerly set in recovery.conf can be set in
postgresql.conf using RELOAD
These parameters will have no defaults in postgresql.conf.sample
Setting them has no effect during normal running, or once recovery ends.
https://www.postgresql.org/docs/devel/static/archive-recovery-settings.html
https://www.postgresql.org/docs/devel/static/recovery-target-settings.html
https://www.postgresql.org/docs/devel/static/standby-settings.html
(Abhijit)

I've attached a WIP patch for the above (recovery_guc_v20160831.patch).
This was based on the unite_recoveryconf_postgresqlconf_v3.patch posted
by Fujii Masao.

Unfortunately, some parts conflict with the patch that Simon just posted
(e.g., his patch removes trigger_file altogether, whereas mine converts
it into a GUC along the lines of the original patch). Rather than trying
to untangle that right now, I'm posting what I have as-is, and I'll post
an updated version tomorrow.

-- Abhijit

On Thu, Sep 1, 2016 at 1:15 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

This is a summary of proposed changes to the recovery.conf API for
v10. These are based in part on earlier discussions, and represent a
minimal modification to current usage.

Proposed changes (with reference to patches from Abhijit Menon-Sen and myself)

* pg_ctl start -M (normal|recover|continue)
pg_ctl can now start the server directly as a standby, similar to the
way pg_ctl promote works. The internal implementation is also similar,
with pg_ctl writing a "recovery.trigger" file that initiates recovery
in the same way recovery.conf used to do. It is still possible to
manually add a file called "recovery.trigger" and have that work if
users desire that mechanism.
Different startup methods can be selected with the <option>-M</option>
option. <quote>Normal</quote> mode starts the server for read/write,
overriding any previous use in Recover mode.
<quote>Recover</quote> mode starts the server as a standby server which
expects to receive changes from a primary/master server using physical
streaming replication or is used for
performing a recovery from backup. <quote>Continue</quote> mode is
the default and will startup the server in whatever mode it was in at
last proper shutdown, or as modified by any trigger files present.
(Patch: recovery_startup_r10_api.v1b.patch)

So you basically just set ArchiveRecoveryRequested based on the
presence of recovery.trigger instead of recovery.conf. And the
interface of pg_ctl:
- recover mode => creates recovery.trigger
- continue mode => does nothing
- normal mode => removes recovery.trigger
That looks like a sound design.

* Recovery parameters would now be part of the main postgresql.conf
infrastructure
Any parameters set in $DATADIR/recovery.conf will be read after the
main parameter file, similar to the way that postgresql.conf.auto is
read.
(Abhijit)
* pg_basebackup -R will continue to generate a parameter file called
recovery.conf as it does now, but will also create a file named
recovery.trigger.
(This part is WIP; patch doesn't include implementation for tar format yet)

Or we could just throw away this dependency with recovery.conf,
simply. I see no need to keep it if recovery is triggered depending on
recovery.trigger, nor recommend its use. We could instead add
include_if_exists 'recovery.conf' at the bottom of postgresql.conf and
let the infrastructure do the rest to simplify the patch.

* Parameters
All of the parameters formerly set in recovery.conf can be set in
postgresql.conf using RELOAD
These parameters will have no defaults in postgresql.conf.sample
Setting them has no effect during normal running, or once recovery ends.
https://www.postgresql.org/docs/devel/static/archive-recovery-settings.html
https://www.postgresql.org/docs/devel/static/recovery-target-settings.html
https://www.postgresql.org/docs/devel/static/standby-settings.html
(Abhijit)

Hm. I think that what would make sense here is a new GUC category,
meaning that once recovery is launched the new parameters are not
taken into account once again. Even updating primary_conninfo would
need a restart of the WAL receiver, so we could just make them
GUC_POSTMASTER and be done with it.

Related cleanup
* Promotion signal file is now called "promote.trigger" rather than
just "promote"

If that's not strictly necessary this renaming is not mandatory.

* Remove user configurable "trigger_file" mechanism - use
"promote.trigger" for all cases

Ugh. I am -1 on that. There are likely backup tools and users that
rely on this option, particularly to be able to trigger promotion
using a file that is on a different partition than PGDATA.

* Remove Fallback promote mechanism, so all promotion is now "fast" in xlog.c

No problem with that. Now others have surely other opinions. That
could be addressed as a separate patch.
--
Michael

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

On Wed, Aug 31, 2016 at 05:15:59PM +0100, Simon Riggs wrote:

<quote>Recover</quote> mode starts the server as a standby server which
expects to receive changes from a primary/master server using physical
streaming replication or is used for performing a recovery from
backup.

I understand where this is coming from, but wouldn't "standby",
"replication" or something be more generally understood than "recover"?
Streaming replication got bolted on to recovery, but that seems like an
implementation detail by now.

Michael

--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.banck@credativ.de

credativ GmbH, HRB M�nchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 M�nchengladbach
Gesch�ftsf�hrung: Dr. Michael Meskes, J�rg Folz, Sascha Heuer

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

On Thu, Sep 1, 2016 at 4:01 AM, Abhijit Menon-Sen <ams@2ndquadrant.com> wrote:

At 2016-08-31 17:15:59 +0100, simon@2ndquadrant.com wrote:

* Recovery parameters would now be part of the main postgresql.conf
infrastructure
Any parameters set in $DATADIR/recovery.conf will be read after the
main parameter file, similar to the way that postgresql.conf.auto is
read.
(Abhijit)

* Parameters
All of the parameters formerly set in recovery.conf can be set in
postgresql.conf using RELOAD
These parameters will have no defaults in postgresql.conf.sample
Setting them has no effect during normal running, or once recovery ends.
https://www.postgresql.org/docs/devel/static/archive-recovery-settings.html
https://www.postgresql.org/docs/devel/static/recovery-target-settings.html
https://www.postgresql.org/docs/devel/static/standby-settings.html
(Abhijit)

I've attached a WIP patch for the above (recovery_guc_v20160831.patch).
This was based on the unite_recoveryconf_postgresqlconf_v3.patch posted
by Fujii Masao.

Unfortunately, some parts conflict with the patch that Simon just posted
(e.g., his patch removes trigger_file altogether, whereas mine converts
it into a GUC along the lines of the original patch). Rather than trying
to untangle that right now, I'm posting what I have as-is, and I'll post
an updated version tomorrow.

- else if (recoveryTarget == RECOVERY_TARGET_XID)
- ereport(LOG,
- (errmsg("starting point-in-time recovery to XID %u",
- recoveryTargetXid)));
User loses information if those logs are removed.

+       {"recovery_min_apply_delay", PGC_SIGHUP, WAL_RECOVERY_TARGET,
+           gettext_noop("Sets the minimum delay to apply changes
during recovery."),
+           NULL,
+           GUC_UNIT_MS
+       },
What's the point of having them all as SIGHUP? The startup process
does not reload GUC parameters with ProcessConfigFile(), it works on
recoveryWakeupLatch though. I'd rather let them as PGC_POSTMASTER to
begin with as that's the safest approach, and then get a second patch
that processes ProcessConfigFile in the startup process and switch
some of them to PGC_SIGHUP. Recovery targets should not be SIGHUP, but
recovery_min_apply_delay applies definitely applies to that, as well
as archive_cleanup_command or restore_command.

+static void
+assign_recovery_target_name(const char *newval, void *extra)
+{
+   if (recovery_target_xid != InvalidTransactionId)
+       recovery_target = RECOVERY_TARGET_XID;
+   else if (newval[0])
+       recovery_target = RECOVERY_TARGET_NAME;
+   else if (recovery_target_time != 0)
+       recovery_target = RECOVERY_TARGET_TIME;
+   else
+       recovery_target = RECOVERY_TARGET_UNSET;
+}
That's brittle to put that in the GUC machinery... The recovery target
type should be determined only once, if archive recovery is wanted at
the beginning of the startup process once and for all, and just be set
up within the startup process. If multiple recovery_target_*
parameters are set, we should just define the target type in order of
priority, instead of the-last-one-wins that is currently present.
-- 
Michael

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

At 2016-09-01 15:51:11 +0900, michael.paquier@gmail.com wrote:

- (errmsg("starting point-in-time recovery to XID %u",
- recoveryTargetXid)));
User loses information if those logs are removed.

Agreed. I'm revising the patch right now, and I decided to leave them.
I'll consider and comment on the remainder of your points after I've
posted an update. Thanks for reading.

-- Abhijit

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

On 1 September 2016 at 06:34, Michael Paquier <michael.paquier@gmail.com> wrote:

On Thu, Sep 1, 2016 at 1:15 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

This is a summary of proposed changes to the recovery.conf API for
v10. These are based in part on earlier discussions, and represent a
minimal modification to current usage.

Proposed changes (with reference to patches from Abhijit Menon-Sen and myself)

* pg_ctl start -M (normal|recover|continue)

...

That looks like a sound design.

Thanks

* Recovery parameters would now be part of the main postgresql.conf
infrastructure
Any parameters set in $DATADIR/recovery.conf will be read after the
main parameter file, similar to the way that postgresql.conf.auto is
read.
(Abhijit)
* pg_basebackup -R will continue to generate a parameter file called
recovery.conf as it does now, but will also create a file named
recovery.trigger.
(This part is WIP; patch doesn't include implementation for tar format yet)

Or we could just throw away this dependency with recovery.conf,
simply. I see no need to keep it if recovery is triggered depending on
recovery.trigger, nor recommend its use. We could instead add
include_if_exists 'recovery.conf' at the bottom of postgresql.conf and
let the infrastructure do the rest to simplify the patch.

It works exactly the same as ALTER SYSTEM and adds only one small hunk of code.

* Parameters
All of the parameters formerly set in recovery.conf can be set in
postgresql.conf using RELOAD
These parameters will have no defaults in postgresql.conf.sample
Setting them has no effect during normal running, or once recovery ends.
https://www.postgresql.org/docs/devel/static/archive-recovery-settings.html
https://www.postgresql.org/docs/devel/static/recovery-target-settings.html
https://www.postgresql.org/docs/devel/static/standby-settings.html
(Abhijit)

Hm. I think that what would make sense here is a new GUC category,
meaning that once recovery is launched the new parameters are not
taken into account once again. Even updating primary_conninfo would
need a restart of the WAL receiver, so we could just make them
GUC_POSTMASTER and be done with it.

We definitely want most of them set at RELOAD, especially recovery targets.

Almost all of them will have no effect when normal mode starts, so I
don't see any other special handling needed.

Related cleanup
* Promotion signal file is now called "promote.trigger" rather than
just "promote"

If that's not strictly necessary this renaming is not mandatory.

I think it makes sense to keep both files with same suffix, for clarity.

* Remove user configurable "trigger_file" mechanism - use
"promote.trigger" for all cases

Ugh. I am -1 on that. There are likely backup tools and users that
rely on this option, particularly to be able to trigger promotion
using a file that is on a different partition than PGDATA.

OK, I wasn't thinking of that. Perhaps we should have a trigger
directory parameter?

* Remove Fallback promote mechanism, so all promotion is now "fast" in xlog.c

No problem with that. Now others have surely other opinions. That
could be addressed as a separate patch.

I'll post separate patch.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

On 31 August 2016 at 20:01, Abhijit Menon-Sen <ams@2ndquadrant.com> wrote:

Unfortunately, some parts conflict with the patch that Simon just posted
(e.g., his patch removes trigger_file altogether, whereas mine converts
it into a GUC along the lines of the original patch). Rather than trying
to untangle that right now, I'm posting what I have as-is, and I'll post
an updated version tomorrow.

Thanks.

archive_cleanup_command no longer needs to be in shmem. Checkpointer
will have its own copy of the value.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

Hi.

Here's an updated version of my patch, which now applies on top of the
patch that Simon posted earlier (recovery_startup_r10_api.v1b.patch).

A few notes:

1. I merged in recovery_target_lsn as a new GUC setting.
2. I fixed various minor nits in the earlier patch, not worth mentioning
individually.
3. I haven't added back trigger_file, because Simon's patch removes it.
I can add it back in separately after discussion (otherwise Simon's
and my patches will conflict).
4. I've tested this to the extent that setting things in postgresql.conf
works, and recovery.conf is still read if it exists, and so on.

One open issue is the various assign_recovery_target_xxx functions,
which Michael noted in his earlier review:

+static void
+assign_recovery_target_xid(const char *newval, void *extra)
+{
+	recovery_target_xid = *((TransactionId *) extra);
+
+	if (recovery_target_xid != InvalidTransactionId)
+		recovery_target = RECOVERY_TARGET_XID;
+	else if (recovery_target_name && *recovery_target_name)
+		recovery_target = RECOVERY_TARGET_NAME;
+	else if (recovery_target_time != 0)
+		recovery_target = RECOVERY_TARGET_TIME;
+	else if (recovery_target_lsn != 0)
+		recovery_target = RECOVERY_TARGET_LSN;
+	else
+		recovery_target = RECOVERY_TARGET_UNSET;
+}

(Note how recovery_target_lsn caused this—and three other functions
besides—to grow an extra branch.)

I don't like this code, but I'm not yet sure what to replace it with. I
think we should address the underlying problem—that the UI doesn't map
cleanly to what the code wants. There's been some discussion about this
earlier, but not any consensus that I could see.

Do we want something like this (easy to implement and document, perhaps
not especially convenient to use):

recovery_target = 'xid' # or 'time'/'name'/'lsn'/'immediate'
recovery_target_xid = xxx? # the only setting we care about now
recovery_target_otherthings = parsed_but_ignored

Or something like this (a bit harder to implement):

recovery_target = 'xid:xxx' # or 'time:xxx' etc.

Alternatively, the do-nothing option is to move the tests from guc.c to
StartupXLOG and do it only once in some defined order (which would also
break the current last-mention-wins behaviour).

Thoughts? (I've added Fujii to the Cc: list, in case he has any
comments, since this is based on his earlier patch.)

-- Abhijit

On Tue, Sep 6, 2016 at 2:18 PM, Abhijit Menon-Sen <ams@2ndquadrant.com> wrote:

One open issue is the various assign_recovery_target_xxx functions,
which Michael noted in his earlier review:
[...]
I don't like this code, but I'm not yet sure what to replace it with. I
think we should address the underlying problem—that the UI doesn't map
cleanly to what the code wants. There's been some discussion about this
earlier, but not any consensus that I could see.

By moving all the recovery parameters to GUC, we will need to define a
hierarchy among the target types. I see no way to avoid that except by
changing the parametrization but that would be more confusing for the
users. So that will not be anymore the last one wins, but the first
one listed wins in all the ones enabled that wins. I think that we
could leverage that a little bit though and reduce the complexity of
the patch: my best advice here is to make all those recovery_target_*
parameters PGC_POSTMASTER so as they are loaded only once when the
server starts, and then we define the recovery target type used in the
startup process instead of trying to do so at GUC level. This does not
change the fact that we'd still need a hierarchy among the target
types, but it slightly reduces the complexity of the patch. And this
does not prevent further enhancements by switching them later to be
PGC_SIGHUP, really. I'd really like to see this improvement, but I
don't think that this applies to this change, which is complicated
enough, and will likely introduce its lot of bugs even after several
reviews.

Do we want something like this (easy to implement and document, perhaps
not especially convenient to use):

recovery_target = 'xid' # or 'time'/'name'/'lsn'/'immediate'
recovery_target_xid = xxx? # the only setting we care about now
recovery_target_otherthings = parsed_but_ignored

Or something like this (a bit harder to implement):

recovery_target = 'xid:xxx' # or 'time:xxx' etc.

Interesting ideas, particularly the last one. Mixing both:
recovery_target_type = 'foo' # can be 'immediate'
recovery_target_value = 'value_of_foo' # does not matter for 'immediate'

Alternatively, the do-nothing option is to move the tests from guc.c to
StartupXLOG and do it only once in some defined order (which would also
break the current last-mention-wins behaviour).

My vote goes in favor of that..

+    else if (recovery_target_lsn != 0)
+        recovery_target = RECOVERY_TARGET_LSN;
This needs to check for InvalidXLogRecPtr.
-- 
Michael

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

At 2016-09-06 14:40:54 +0900, michael.paquier@gmail.com wrote:

my best advice here is to make all those recovery_target_* parameters
PGC_POSTMASTER so as they are loaded only once when the server starts,
and then we define the recovery target type used in the startup
process instead of trying to do so at GUC level.

I understand your approach in light of the GUC code, but I see things a
bit differently—the complexity comes largely from the specific handling
of recovery_target. I'll try to come up with a way to do it better. If
not, we have your suggestion to fall back on.

+    else if (recovery_target_lsn != 0)
+        recovery_target = RECOVERY_TARGET_LSN;
This needs to check for InvalidXLogRecPtr.

Of course, thanks. Fixed locally in all the relevant places. Will repost
whenever there are other substantive changes.

-- Abhijit

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

On 6 September 2016 at 08:06, Abhijit Menon-Sen <ams@2ndquadrant.com> wrote:

At 2016-09-06 14:40:54 +0900, michael.paquier@gmail.com wrote:

my best advice here is to make all those recovery_target_* parameters
PGC_POSTMASTER so as they are loaded only once when the server starts,
and then we define the recovery target type used in the startup
process instead of trying to do so at GUC level.

I understand your approach in light of the GUC code, but I see things a
bit differently—the complexity comes largely from the specific handling
of recovery_target. I'll try to come up with a way to do it better. If
not, we have your suggestion to fall back on.

As I said upthread...
"We definitely want most of them set at RELOAD, especially recovery targets."
So PGC_POSTMASTER is not the objective.

How then to proceed?

I guess we could keep the old parameters and make them PGC_POSTMASTER,
but also provide a new parameter called recovery_target that
simplifies the API and is PGC_SIGHUP. That way we resolve the
annoyance of handling the current ones but keep compatibility for
those who can't move on, just yet.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

On 06/09/16 07:18, Abhijit Menon-Sen wrote:

One open issue is the various assign_recovery_target_xxx functions,
which Michael noted in his earlier review:

+static void
+assign_recovery_target_xid(const char *newval, void *extra)
+{
+	recovery_target_xid = *((TransactionId *) extra);
+
+	if (recovery_target_xid != InvalidTransactionId)
+		recovery_target = RECOVERY_TARGET_XID;
+	else if (recovery_target_name && *recovery_target_name)
+		recovery_target = RECOVERY_TARGET_NAME;
+	else if (recovery_target_time != 0)
+		recovery_target = RECOVERY_TARGET_TIME;
+	else if (recovery_target_lsn != 0)
+		recovery_target = RECOVERY_TARGET_LSN;
+	else
+		recovery_target = RECOVERY_TARGET_UNSET;
+}

(Note how recovery_target_lsn caused this�and three other functions
besides�to grow an extra branch.)

I don't like this code, but I'm not yet sure what to replace it with. I
think we should address the underlying problem�that the UI doesn't map
cleanly to what the code wants. There's been some discussion about this
earlier, but not any consensus that I could see.

Do we want something like this (easy to implement and document, perhaps
not especially convenient to use):

recovery_target = 'xid' # or 'time'/'name'/'lsn'/'immediate'
recovery_target_xid = xxx? # the only setting we care about now
recovery_target_otherthings = parsed_but_ignored

Or something like this (a bit harder to implement):

recovery_target = 'xid:xxx' # or 'time:xxx' etc.

Personally, I never liked the fact that we have several config variables
for this and then the last one is chosen (even when it was in
recovery.conf). We support one recovery_target at a time so it would
make sense to have single config option for it IMHO.

So +1 on the recovery_target = 'xid:xxx' idea.

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

At 2016-09-06 10:56:53 +0200, petr@2ndquadrant.com wrote:

So +1 on the recovery_target = 'xid:xxx' idea.

OK, I'll make it work that way. New patch in a couple of days.

-- Abhijit

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

On 9/6/16 4:56 AM, Petr Jelinek wrote:

On 06/09/16 07:18, Abhijit Menon-Sen wrote:

Do we want something like this (easy to implement and document, perhaps
not especially convenient to use):

recovery_target = 'xid' # or 'time'/'name'/'lsn'/'immediate'
recovery_target_xid = xxx? # the only setting we care about now
recovery_target_otherthings = parsed_but_ignored

Or something like this (a bit harder to implement):

recovery_target = 'xid:xxx' # or 'time:xxx' etc.

Personally, I never liked the fact that we have several config variables
for this and then the last one is chosen (even when it was in
recovery.conf). We support one recovery_target at a time so it would
make sense to have single config option for it IMHO.

So +1 on the recovery_target = 'xid:xxx' idea.

I would rather not combine the type and target into a single field - how
about having two fields:

recovery_target_type = 'xid|time|name|immediate'
recovery_target = 'value'

recovery_target would be ignored when recovery_target_type = 'immediate'.

--
-David
david@pgmasters.net

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

On Wed, Aug 31, 2016 at 9:45 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

This is a summary of proposed changes to the recovery.conf API for
v10. These are based in part on earlier discussions, and represent a
minimal modification to current usage.

This looks great.

Proposed changes (with reference to patches from Abhijit Menon-Sen and myself)

* pg_ctl start -M (normal|recover|continue)
pg_ctl can now start the server directly as a standby, similar to the
way pg_ctl promote works. The internal implementation is also similar,
with pg_ctl writing a "recovery.trigger" file that initiates recovery
in the same way recovery.conf used to do. It is still possible to
manually add a file called "recovery.trigger" and have that work if
users desire that mechanism.
Different startup methods can be selected with the <option>-M</option>
option. <quote>Normal</quote> mode starts the server for read/write,
overriding any previous use in Recover mode.
<quote>Recover</quote> mode starts the server as a standby server which
expects to receive changes from a primary/master server using physical
streaming replication or is used for
performing a recovery from backup. <quote>Continue</quote> mode is
the default and will startup the server in whatever mode it was in at
last proper shutdown, or as modified by any trigger files present.
(Patch: recovery_startup_r10_api.v1b.patch)

I like this. I think your choice of naming is good, too. Michael
suggested something else downthread, but I think it's good to call
this "recover" because we don't know whether the user is doing
replication or PITR or whatever; "recover" is the internal name and is
fairly well-understood database jargon.

* $DATADIR/recovery.conf no longer triggers recovery
(Patch: recovery_startup_r10_api.v1b.patch)

* Recovery parameters would now be part of the main postgresql.conf
infrastructure
Any parameters set in $DATADIR/recovery.conf will be read after the
main parameter file, similar to the way that postgresql.conf.auto is
read.
(Abhijit)

Makes sense.

* pg_basebackup -R will continue to generate a parameter file called
recovery.conf as it does now, but will also create a file named
recovery.trigger.
(This part is WIP; patch doesn't include implementation for tar format yet)

Hmm, OK.

* Parameters
All of the parameters formerly set in recovery.conf can be set in
postgresql.conf using RELOAD
These parameters will have no defaults in postgresql.conf.sample
Setting them has no effect during normal running, or once recovery ends.
https://www.postgresql.org/docs/devel/static/archive-recovery-settings.html
https://www.postgresql.org/docs/devel/static/recovery-target-settings.html
https://www.postgresql.org/docs/devel/static/standby-settings.html
(Abhijit)

This sounds really good.

Related cleanup
* Promotion signal file is now called "promote.trigger" rather than
just "promote"
* Remove user configurable "trigger_file" mechanism - use
"promote.trigger" for all cases

I'm in favor of this. I don't think that it's very hard for authors
of backup tools to adapt to this new world, and I don't see that
allowing configurability here does anything other than create more
cases to worry about.

As you can see, I don't have a lot of substance to add, but I wanted
to lend my +1 to all of these proposals, which I think will improve
ease of use and probably lead to easier code maintenance, too.
Thanks!

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

On 06/09/16 13:52, David Steele wrote:

On 9/6/16 4:56 AM, Petr Jelinek wrote:

On 06/09/16 07:18, Abhijit Menon-Sen wrote:

Do we want something like this (easy to implement and document, perhaps
not especially convenient to use):

recovery_target = 'xid' # or 'time'/'name'/'lsn'/'immediate'
recovery_target_xid = xxx? # the only setting we care about now
recovery_target_otherthings = parsed_but_ignored

Or something like this (a bit harder to implement):

recovery_target = 'xid:xxx' # or 'time:xxx' etc.

Personally, I never liked the fact that we have several config variables
for this and then the last one is chosen (even when it was in
recovery.conf). We support one recovery_target at a time so it would
make sense to have single config option for it IMHO.

So +1 on the recovery_target = 'xid:xxx' idea.

I would rather not combine the type and target into a single field - how
about having two fields:

recovery_target_type = 'xid|time|name|immediate'
recovery_target = 'value'

recovery_target would be ignored when recovery_target_type = 'immediate'.

That's also reasonable solution, I don't really have preference between
those. My main point was to get rid of the 5 or so variables where only
one will actually be used in the end.

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

On Tue, Sep 6, 2016 at 10:01 PM, Petr Jelinek <petr@2ndquadrant.com> wrote:

That's also reasonable solution, I don't really have preference between
those. My main point was to get rid of the 5 or so variables where only one
will actually be used in the end.

And no need to worry about the priority of the target types here. The
last value specified wins.
--
Michael

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

On 9/6/16 8:07 AM, Robert Haas wrote:

On Wed, Aug 31, 2016 at 9:45 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

Related cleanup
* Promotion signal file is now called "promote.trigger" rather than
just "promote"
* Remove user configurable "trigger_file" mechanism - use
"promote.trigger" for all cases

I'm in favor of this. I don't think that it's very hard for authors
of backup tools to adapt to this new world, and I don't see that
allowing configurability here does anything other than create more
cases to worry about.

+1 from a backup tool author.

--
-David
david@pgmasters.net

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

On Tue, Sep 6, 2016 at 10:11 AM, David Steele <david@pgmasters.net> wrote:

On 9/6/16 8:07 AM, Robert Haas wrote:

On Wed, Aug 31, 2016 at 9:45 PM, Simon Riggs <simon@2ndquadrant.com>
wrote:

Related cleanup
* Promotion signal file is now called "promote.trigger" rather than
just "promote"
* Remove user configurable "trigger_file" mechanism - use
"promote.trigger" for all cases

I'm in favor of this. I don't think that it's very hard for authors
of backup tools to adapt to this new world, and I don't see that
allowing configurability here does anything other than create more
cases to worry about.

+1 from a backup tool author.

It's time to wrap up this CommitFest, and this thread doesn't seem to
contain anything that looks like a committable patch. So, I'm marking
this "Returned with Feedback". I hope that the fact that there's been
no discussion for the last three weeks doesn't mean this effort is
dead; I would like very much to see it move forward.

Thanks,

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