Thread: Proposal for changes to recovery.conf API

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)

* $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)

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

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

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
* Remove Fallback promote mechanism, so all promotion is now "fast" in xlog.c
* Rename CheckForStandbyTrigger() to CheckForPromoteTrigger() for clarity
(Patch: recovery_startup_r10_api.v1b.patch)

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

On 09/28/2016 10:13 AM, Robert Haas wrote:
> 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.

Has this gone anywhere?  Given that we're in "break all the things" mode
for PostgreSQL 10, it would be the ideal time to consolidate
recovery.conf with pg.conf.


-- 
--
Josh Berkus
Red Hat OSAS
(any opinions are my own)

At 2016-09-28 13:13:56 -0400, robertmhaas@gmail.com wrote:
>
> 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.

Here's an updated patch. Sorry, I got busy elswhere.

I struggled with the handling of recovery_target a little. For example,
one suggested alternative was:

    recovery_target_type = xid
    recovery_target_value = …

The problem with implementing it this way is that the _value setting
cannot be parsed without already having parsed the _type, and I didn't
want to force that sort of dependency.

What I've done instead is to make this work:

    recovery_target = xid|time|name|lsn|immediate
    recovery_target_xid = …
    recovery_target_time = …
    recovery_target_name = …
    recovery_target_lsn = …

The recovery_target_xxx values are parsed as they used to be, but the
one that's used is the one that's set in recovery_target. That's easy to
explain, and the patch is much less intrusive, but I'm certainly open to
suggestions to improve this, and I have the time to work on this patch
with a view towards getting it committed in this cycle.

-- Abhijit

P.S. Sorry, I haven't been able to resolve the conflicts between Simon's
earlier recovery_startup_r10_api.v1b patch and the "pg_ctl promote -w"
changes in master. I was distracted by some illness in the family, but
I will post another update very soon.

On Mon, Oct 31, 2016 at 7:44 PM, Abhijit Menon-Sen <ams@2ndquadrant.com> wrote:
> At 2016-09-28 13:13:56 -0400, robertmhaas@gmail.com wrote:
>>>> 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.
>
> Here's an updated patch. Sorry, I got busy elswhere.
>
> I struggled with the handling of recovery_target a little. For example,
> one suggested alternative was:
>
>     recovery_target_type = xid
>     recovery_target_value = …
>
> The problem with implementing it this way is that the _value setting
> cannot be parsed without already having parsed the _type, and I didn't
> want to force that sort of dependency.
>
> What I've done instead is to make this work:
>
>     recovery_target = xid|time|name|lsn|immediate
>     recovery_target_xid = …
>     recovery_target_time = …
>     recovery_target_name = …
>     recovery_target_lsn = …
>
> The recovery_target_xxx values are parsed as they used to be, but the
> one that's used is the one that's set in recovery_target. That's easy to
> explain, and the patch is much less intrusive, but I'm certainly open to
> suggestions to improve this, and I have the time to work on this patch
> with a view towards getting it committed in this cycle.

I liked Heikki's suggestion (at some point quite a while ago now) of
recovery_target = 'xid 123' or recovery_target='lsn 0/723' or
whatever.

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

On Tue, Nov 1, 2016 at 11:31 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> I liked Heikki's suggestion (at some point quite a while ago now) of
> recovery_target = 'xid 123' or recovery_target='lsn 0/723' or
> whatever.

My vote goes for having two separate parameters, because as we know
that there will be always two fields in this parameter, there is no
need to complicate the GUC machinery with a new special case when
parsing its value. Having two parameters would also make easier the
life of anybody maintaining a library parsing parameters for values
and doing in-place updates of those values. For example, I maintain a
set of routines in Python doing that with some fancy regex routines,
and that would avoid having to handle a special case when willing to
update for example the same recovery target with a new value.
-- 
Michael

On 4 November 2016 at 10:04, Michael Paquier <michael.paquier@gmail.com> wrote:
> On Tue, Nov 1, 2016 at 11:31 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> I liked Heikki's suggestion (at some point quite a while ago now) of
>> recovery_target = 'xid 123' or recovery_target='lsn 0/723' or
>> whatever.
>
> My vote goes for having two separate parameters, because as we know
> that there will be always two fields in this parameter, there is no
> need to complicate the GUC machinery with a new special case when
> parsing its value. Having two parameters would also make easier the
> life of anybody maintaining a library parsing parameters for values
> and doing in-place updates of those values. For example, I maintain a
> set of routines in Python doing that with some fancy regex routines,
> and that would avoid having to handle a special case when willing to
> update for example the same recovery target with a new value.

Parameters are required to all make sense independently, so two
parameters is off the table, IMHO.

The choice is one parameter, as Robert mentions again, or lots of
parameters (or both of those).

Since the "lots of parameters" approach is almost exactly what we have
now, I think we should do that first, get this patch committed and
then sit back and discuss an improved API and see what downsides it
introduces. Further delay on this isn't helpful for other patches
going in.

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

On Fri, Nov 4, 2016 at 6:04 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Tue, Nov 1, 2016 at 11:31 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> I liked Heikki's suggestion (at some point quite a while ago now) of
>> recovery_target = 'xid 123' or recovery_target='lsn 0/723' or
>> whatever.
>
> My vote goes for having two separate parameters, because as we know
> that there will be always two fields in this parameter, ...

That's not even true today: when the target is immediate, it has no
associated parameter value.  And who knows what the future may hold?

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

At 2016-11-04 10:35:05 +0000, simon@2ndquadrant.com wrote:
>
> Since the "lots of parameters" approach is almost exactly what we have
> now, I think we should do that first, get this patch committed and
> then sit back and discuss an improved API and see what downsides it
> introduces.

Thanks, I agree strongly with this.

Someone (Michael?) earlier mentioned the potential for introducing bugs
with this patch (the idea of merging recovery.conf into postgresql.conf
at all, not this particular incarnation).

I think the current proposed approach with
   recovery_target=xid   recovery_target_xid=xxx

is preferable because (a) it doesn't introduce much new code, e.g., new
parsing functions, nor (b) need many changes in the documentation—all we
need is to say that of the various recovery_target_xxx parameters, the
one that has priority is the one named by recovery_target.

If I were to introduce recovery_target='xid xxx', I would at a minimum
need to factor out (or duplicate) parsing and error handling code, make
a type/value union-in-struct to store in the GUC *extra, then make sure
that we handle the older values in a backwards-compatible way, and move
a bunch of documentation around.

Can it be done? Yes, of course; and I'll do it if that's the consensus.
But it would be a backwards-compatible change to the current approach
anyway, and I think it would be better to put in the simple way now
before discussing the new API further.

Let's get the basic new *functionality* out so that people can play with
it, I'm sure we'll find a few non-API things that need tweaking as a
result of the change.

-- Abhijit

Hi,

On 2016-11-01 05:14:58 +0530, Abhijit Menon-Sen wrote:
> Subject: Convert recovery.conf settings to GUCs

I really want to get this in, we've been back and forth about this for
far too long.

>     <para>
>-     Create a recovery command file <filename>recovery.conf</> in the cluster
>-     data directory (see <xref linkend="recovery-config">). You might
>+     Set up recovery parameters in <filename>postgresql.conf</> (see
>+     <xref linkend="runtime-config-wal-archive-recovery"> and
>+     <xref linkend="runtime-config-wal-recovery-target">).
>+    </para>
>+   </listitem>
>+   <listitem>
>+    <para>
>+     Create a recovery trigger file <filename>recovery.trigger</>
>+     in the cluster data directory. You might
>      also want to temporarily modify <filename>pg_hba.conf</> to prevent
>      ordinary users from connecting until you are sure the recovery was successful.
>     </para>

I think this means we need to rephrase a number of docs and code
references to trigger files, because they'll currently often refer to
the promotion trigger, no?


> diff --git a/src/backend/access/transam/recovery.conf.sample b/src/backend/access/transam/recovery.conf.sample
> index 7a16751..15ce784 100644
> --- a/src/backend/access/transam/recovery.conf.sample
> +++ b/src/backend/access/transam/recovery.conf.sample
> @@ -2,23 +2,14 @@
>  # PostgreSQL recovery config file
>  # -------------------------------
>  #
> -# Edit this file to provide the parameters that PostgreSQL needs to
> -# perform an archive recovery of a database, or to act as a replication
> -# standby.
> +# PostgreSQL 10.0 or later, recovery.conf is no longer used. Instead,
> +# specify all recovery parameters in postgresql.conf and create
> +# recovery.trigger to enter archive recovery or standby mode.
>  #
> -# If "recovery.conf" is present in the PostgreSQL data directory, it is
> -# read on postmaster startup.  After successful recovery, it is renamed
> -# to "recovery.done" to ensure that we do not accidentally re-enter
> -# archive recovery or standby mode.
> +# If you must use recovery.conf, specify "include directives" in
> +# postgresql.conf like this:
>  #
> -# This file consists of lines of the form:
> -#
> -#   name = value
> -#
> -# Comments are introduced with '#'.
> -#
> -# The complete list of option names and allowed values can be found
> -# in the PostgreSQL documentation.
> +#   include 'recovery.conf'

This should probably warn about the differences in "trigger" behaviour
of recovery.conf being present.

>  #---------------------------------------------------------------------------
>  # ARCHIVE RECOVERY PARAMETERS
> @@ -131,14 +122,6 @@
>  #
>  #primary_slot_name = ''

I don't think it makes much sense to still list all this?


> +bool        standby_mode = false;

I'm very tempted to rename this during the move to GUCs.


> +char       *primary_conninfo = NULL;
> +char       *primary_slot_name = NULL;

Slightly less so, but still tempted to also rename these. They're not
actually necessarily pointing towards a primary, and namespace-wise
they're not grouped with recovery_*, which has become more important now
that recovery.conf isn't a separate namespace anymore.

> -static int    recovery_min_apply_delay = 0;
>  static TimestampTz recoveryDelayUntilTime;
>
> +static TimestampTz recoveryDelayUntilTime;
> +

Isn't this a repeated definition?

>  /*
>   * During normal operation, the only timeline we care about is ThisTimeLineID.
>   * During recovery, however, things are more complicated.  To simplify life
> @@ -577,10 +578,10 @@ typedef struct XLogCtlData
>      TimeLineID    PrevTimeLineID;
>
>      /*
> -     * archiveCleanupCommand is read from recovery.conf but needs to be in
> -     * shared memory so that the checkpointer process can access it.
> +     * archive_cleanup_command is read from recovery.conf but needs to
> +     * be in shared memory so that the checkpointer process can access it.
>       */

References recovery.conf. It'd be a good idea to grep for all remaining
references to recovery.conf.


> +static bool
> +check_recovery_target(char **newval, void **extra, GucSource source)
> +{
> +    RecoveryTargetType *myextra;
> +    RecoveryTargetType rt = RECOVERY_TARGET_UNSET;
> +
> +    if (strcmp(*newval, "xid") == 0)
> +        rt = RECOVERY_TARGET_XID;
> +    else if (strcmp(*newval, "time") == 0)
> +        rt = RECOVERY_TARGET_TIME;
> +    else if (strcmp(*newval, "name") == 0)
> +        rt = RECOVERY_TARGET_NAME;
> +    else if (strcmp(*newval, "lsn") == 0)
> +        rt = RECOVERY_TARGET_LSN;
> +    else if (strcmp(*newval, "immediate") == 0)
> +        rt = RECOVERY_TARGET_IMMEDIATE;
> +    else if (strcmp(*newval, "") != 0)
> +    {
> +        GUC_check_errdetail("recovery_target is not valid: \"%s\"", *newval);
> +        return false;
> +    }
> +
> +    myextra = (RecoveryTargetType *) guc_malloc(ERROR, sizeof(RecoveryTargetType));
> +    *myextra = rt;
> +    *extra = (void *) myextra;
> +
> +    return true;
> +}

Shouldn't this instead be an enum type GUC?

> +static bool
> +check_recovery_target_time(char **newval, void **extra, GucSource source)
> +{
> +    TimestampTz     time;
> +    TimestampTz     *myextra;
> +    MemoryContext oldcontext = CurrentMemoryContext;
> +
> +    PG_TRY();
> +    {
> +        time = (strcmp(*newval, "") == 0) ?
> +            0 :
> +            DatumGetTimestampTz(DirectFunctionCall3(timestamptz_in,
> +                                                    CStringGetDatum(*newval),
> +                                                    ObjectIdGetDatum(InvalidOid),
> +                                                    Int32GetDatum(-1)));
> +    }
> +    PG_CATCH();
> +    {
> +        ErrorData  *edata;
> +
> +        /* Save error info */
> +        MemoryContextSwitchTo(oldcontext);
> +        edata = CopyErrorData();
> +        FlushErrorState();
> +
> +        /* Pass the error message */
> +        GUC_check_errdetail("%s", edata->message);
> +        FreeErrorData(edata);
> +        return false;
> +    }
> +    PG_END_TRY();

Hm, I'm not happy to do that kind of thing. What if there's ever any
database interaction added to timestamptz_in?

It's also problematic because the parsing of timestamps depends on the
timezone GUC - which might not yet have taken effect...


> +static bool
> +check_recovery_target_lsn(char **newval, void **extra, GucSource source)
> +{
> +    XLogRecPtr        lsn;
> +    XLogRecPtr       *myextra;
> +    MemoryContext oldcontext = CurrentMemoryContext;
> +
> +    PG_TRY();
> +    {
> +        lsn = (strcmp(*newval, "") == 0) ?
> +            InvalidXLogRecPtr :
> +            DatumGetLSN(DirectFunctionCall3(pg_lsn_in,
> +                                            CStringGetDatum(*newval),
> +                                            ObjectIdGetDatum(InvalidOid),
> +                                            Int32GetDatum(-1)));
> +    }
> +    PG_CATCH();
> +    {
> +        ErrorData  *edata;
> +
> +        /* Save error info */
> +        MemoryContextSwitchTo(oldcontext);
> +        edata = CopyErrorData();
> +        FlushErrorState();
> +
> +        /* Pass the error message */
> +        GUC_check_errdetail("%s", edata->message);
> +        FreeErrorData(edata);
> +        return false;
> +    }
> +    PG_END_TRY();

That seems like a pretty pointless use of pg_lsn_in, given the problems
that PG_CATCHing an error without rethrowing brings.


> +static bool
> +check_recovery_target_action(char **newval, void **extra, GucSource source)
> +{
> +    RecoveryTargetAction rta = RECOVERY_TARGET_ACTION_PAUSE;
> +    RecoveryTargetAction *myextra;
> +
> +    if (strcmp(*newval, "pause") == 0)
> +        rta = RECOVERY_TARGET_ACTION_PAUSE;
> +    else if (strcmp(*newval, "promote") == 0)
> +        rta = RECOVERY_TARGET_ACTION_PROMOTE;
> +    else if (strcmp(*newval, "shutdown") == 0)
> +        rta = RECOVERY_TARGET_ACTION_SHUTDOWN;
> +    else if (strcmp(*newval, "") != 0)
> +    {
> +        GUC_check_errdetail("recovery_target_action is not valid: \"%s\"", *newval);
> +        return false;
> +    }
> +
> +    myextra = (RecoveryTargetAction *) guc_malloc(ERROR, sizeof(RecoveryTargetAction));
> +    *myextra = rta;
> +    *extra = (void *) myextra;
> +
> +    return true;
> +}

Should be an enum.


> +static bool
> +check_primary_slot_name(char **newval, void **extra, GucSource source)
> +{
> +    if (strcmp(*newval,"") != 0 &&
> +        !ReplicationSlotValidateName(*newval, WARNING))
> +    {
> +        GUC_check_errdetail("primary_slot_name is not valid: \"%s\"", *newval);
> +        return false;
> +    }
> +
> +    return true;
> +}

ReplicationSlotValidateName will emit WARNINGs this way - hm. That'll
often loose detail about what the problem actually is, e.g. at server
startup.

> +#standby_mode = off            # "on" starts the server as a standby
> +                    # (change requires restart)
> +#primary_conninfo = ''            # connection string to connect to the master
> +#trigger_file = ''            # trigger file to promote the standby

Too general a name imo.


Greetings,

Andres Freund

On Fri, Nov 4, 2016 at 10:17 AM, Abhijit Menon-Sen <ams@2ndquadrant.com> wrote:
> At 2016-11-04 10:35:05 +0000, simon@2ndquadrant.com wrote:
>> Since the "lots of parameters" approach is almost exactly what we have
>> now, I think we should do that first, get this patch committed and
>> then sit back and discuss an improved API and see what downsides it
>> introduces.
>
> Thanks, I agree strongly with this.
>
> Someone (Michael?) earlier mentioned the potential for introducing bugs
> with this patch (the idea of merging recovery.conf into postgresql.conf
> at all, not this particular incarnation).
>
> I think the current proposed approach with
>
>     recovery_target=xid
>     recovery_target_xid=xxx
>
> is preferable because (a) it doesn't introduce much new code, e.g., new
> parsing functions, nor (b) need many changes in the documentation—all we
> need is to say that of the various recovery_target_xxx parameters, the
> one that has priority is the one named by recovery_target.
>
> If I were to introduce recovery_target='xid xxx', I would at a minimum
> need to factor out (or duplicate) parsing and error handling code, make
> a type/value union-in-struct to store in the GUC *extra, then make sure
> that we handle the older values in a backwards-compatible way, and move
> a bunch of documentation around.

Just to be clear, the sum total of the additional "parsing" we are
talking about is looking for the first sequence of 1 or more spaces in
the input string and separating the stuff before them from the stuff
after them.  I agree that there's some work there, but I'm surprised
to hear that you think it's a lot of work.

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

On Sat, Nov 12, 2016 at 11:09 AM, Andres Freund <andres@anarazel.de> wrote:
> I'm very tempted to rename this during the move to GUCs
...
> Slightly less so, but still tempted to also rename these. They're not
> actually necessarily pointing towards a primary, and namespace-wise
> they're not grouped with recovery_*, which has become more important now
> that recovery.conf isn't a separate namespace anymore.

-1 for renaming these.  I don't think the current names are
particularly bad, and I think trying to agree on what would be better
could easily sink the whole patch.

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

On 14 November 2016 at 15:50, Robert Haas <robertmhaas@gmail.com> wrote:
> On Sat, Nov 12, 2016 at 11:09 AM, Andres Freund <andres@anarazel.de> wrote:
>> I'm very tempted to rename this during the move to GUCs
> ...
>> Slightly less so, but still tempted to also rename these. They're not
>> actually necessarily pointing towards a primary, and namespace-wise
>> they're not grouped with recovery_*, which has become more important now
>> that recovery.conf isn't a separate namespace anymore.
>
> -1 for renaming these.  I don't think the current names are
> particularly bad, and I think trying to agree on what would be better
> could easily sink the whole patch.

OK, so we can move forward. Thanks.

I'm going to be doing final review and commit this week, at the
Developer meeting on Thurs and on Friday, with input in person and on
list.

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

On 29 November 2016 at 15:13, Simon Riggs <simon@2ndquadrant.com> wrote:
> On 14 November 2016 at 15:50, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Sat, Nov 12, 2016 at 11:09 AM, Andres Freund <andres@anarazel.de> wrote:
>>> I'm very tempted to rename this during the move to GUCs
>> ...
>>> Slightly less so, but still tempted to also rename these. They're not
>>> actually necessarily pointing towards a primary, and namespace-wise
>>> they're not grouped with recovery_*, which has become more important now
>>> that recovery.conf isn't a separate namespace anymore.
>>
>> -1 for renaming these.  I don't think the current names are
>> particularly bad, and I think trying to agree on what would be better
>> could easily sink the whole patch.
>
> OK, so we can move forward. Thanks.
>
> I'm going to be doing final review and commit this week, at the
> Developer meeting on Thurs and on Friday, with input in person and on
> list.

err... no I'm not, based on review feedback in Tokyo.

New schedule is roughly this...
* agree changes over next week
* make changes and submit new patch by 1 Jan
* commit patch by 7 Jan

Overview of details agreed in Tokyo, now subject to further comments
from hackers

* Move recovery.conf parameters into postgresql.conf
Allow reload of most parameters, allow ALTER SYSTEM
Provide visibility of values through GUC interface

* recovery.conf is replaced by   recovery.trigger -> recovery.done

* pg_basebackup -R
will write recovery.trigger to data directory
insert parameters postgresql.conf.auto, if possible

* backwards compatibility - read recovery.conf from $DATADIR -
presence of recovery.conf will cause ERROR
* backwards compatibility - some parameter names will change, so
allows others to changes also if needed

* Add docs: "Guide to changes in recovery.conf in 10.0"

* recovery_target as a single parameter, using proposed "xid 666"
"target value" format

* remove hot_standby parameter altogether, in line with earlier changes

* trigger_file renamed to promote_trigger_file

* standby_mode = on| off -> default to on

* pg_ctl recover - not as part of this patch

I'll work on the patch from here on, to allow me to work towards commit.

Comments please.

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

On Thu, Dec 1, 2016 at 7:31 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
> * Move recovery.conf parameters into postgresql.conf
> Allow reload of most parameters, allow ALTER SYSTEM
> Provide visibility of values through GUC interface

+1.

> * recovery.conf is replaced by   recovery.trigger -> recovery.done

+1.

> * pg_basebackup -R
> will write recovery.trigger to data directory
> insert parameters postgresql.conf.auto, if possible

Don't understand that last line; otherwise, +1.

> * backwards compatibility - read recovery.conf from $DATADIR -
> presence of recovery.conf will cause ERROR

+1.

> * backwards compatibility - some parameter names will change, so
> allows others to changes also if needed

Don't understand this.

> * Add docs: "Guide to changes in recovery.conf in 10.0"

Hmm, we don't usually write the docs in terms of how things are
different from a previous version.  Might seem strange in 5 years.
Not sure what's best, here.

> * recovery_target as a single parameter, using proposed "xid 666"
> "target value" format

+1.

> * remove hot_standby parameter altogether, in line with earlier changes

That seems a little surprising.  We don't think anyone ever wants to
refuse connections during archive recovery?

> * trigger_file renamed to promote_trigger_file

Why?

> * standby_mode = on| off -> default to on

+1.

> * pg_ctl recover - not as part of this patch

+1.

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

On Fri, Dec 2, 2016 at 10:10 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Dec 1, 2016 at 7:31 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
>> * pg_basebackup -R
>> will write recovery.trigger to data directory
>> insert parameters postgresql.conf.auto, if possible
>
> Don't understand that last line; otherwise, +1.

pg_basebackup -R creates a recovery.conf now, by appending the
parameters to postgresql.conf.auto we are sure that:
1) there is no need to check for the existence of recovery.conf as it
could be included by postgresql.conf with something like an
include_if_exists
2) postgresql.conf.auto is loaded automatically without any additional
tweaks needed in the GUC parsing code paths.

>> * Add docs: "Guide to changes in recovery.conf in 10.0"
>
> Hmm, we don't usually write the docs in terms of how things are
> different from a previous version.  Might seem strange in 5 years.
> Not sure what's best, here.

A good chunk in the release notes would make sense as well?

>> * recovery_target as a single parameter, using proposed "xid 666"
>> "target value" format
>
> +1.
>
>> * remove hot_standby parameter altogether, in line with earlier changes
>
> That seems a little surprising.  We don't think anyone ever wants to
> refuse connections during archive recovery?

I suggested that yesterday. We have talked as well about merging
standby_mode with hot_standby, but at the end most use cases I have
seen involve looking at pg_is_in_recovery() these days to determine if
a node is out of recovery of not, and this makes particularly more
sense since 9.6 where wal_level = archive <=> hot_standby. The thought
behind that is also partially that people complain that replication is
too hard to understand for people.

>> * trigger_file renamed to promote_trigger_file
>
> Why?

Because this is a trigger file aimed at doing promotion, not something else.
-- 
Michael

On Thu, Dec 1, 2016 at 8:28 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Fri, Dec 2, 2016 at 10:10 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Thu, Dec 1, 2016 at 7:31 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
>>> * pg_basebackup -R
>>> will write recovery.trigger to data directory
>>> insert parameters postgresql.conf.auto, if possible
>>
>> Don't understand that last line; otherwise, +1.
>
> pg_basebackup -R creates a recovery.conf now, by appending the
> parameters to postgresql.conf.auto we are sure that:
> 1) there is no need to check for the existence of recovery.conf as it
> could be included by postgresql.conf with something like an
> include_if_exists
> 2) postgresql.conf.auto is loaded automatically without any additional
> tweaks needed in the GUC parsing code paths.

Well, as to #1, we're making that an error IIUC.  But I see the point
of #2, for sure.  So sounds good to me.

>>> * Add docs: "Guide to changes in recovery.conf in 10.0"
>>
>> Hmm, we don't usually write the docs in terms of how things are
>> different from a previous version.  Might seem strange in 5 years.
>> Not sure what's best, here.
>
> A good chunk in the release notes would make sense as well?

Or instead.

>>> * remove hot_standby parameter altogether, in line with earlier changes
>>
>> That seems a little surprising.  We don't think anyone ever wants to
>> refuse connections during archive recovery?
>
> I suggested that yesterday. We have talked as well about merging
> standby_mode with hot_standby, but at the end most use cases I have
> seen involve looking at pg_is_in_recovery() these days to determine if
> a node is out of recovery of not, and this makes particularly more
> sense since 9.6 where wal_level = archive <=> hot_standby. The thought
> behind that is also partially that people complain that replication is
> too hard to understand for people.

If it were up to me, I'd keep the hot_standby parameter around but
default it to 'on'.

>>> * trigger_file renamed to promote_trigger_file
>>
>> Why?
>
> Because this is a trigger file aimed at doing promotion, not something else.

Oh, I see.  Makes sense.  I was confused.

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

On Mon, Dec 5, 2016 at 11:34 AM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
> As there was a feedback from others related to the patch and doesn't find
> any
> update from author.
>
> Closed in 2016-11 commitfest with "returned with feedback" status.
> Please feel free to update the status once you submit the updated patch or
> if the current status doesn't reflect the actual status of the patch.

Having a consensus here is already a great step forward. I am sure
that a new version will be sent for the next CF.
-- 
Michael

On 12/04/2016 07:21 PM, Michael Paquier wrote:
> On Mon, Dec 5, 2016 at 11:34 AM, Haribabu Kommi
> <kommi.haribabu@gmail.com> wrote:
>> As there was a feedback from others related to the patch and doesn't find
>> any
>> update from author.
>>
>> Closed in 2016-11 commitfest with "returned with feedback" status.
>> Please feel free to update the status once you submit the updated patch or
>> if the current status doesn't reflect the actual status of the patch.
> 
> Having a consensus here is already a great step forward. I am sure
> that a new version will be sent for the next CF.
> 

Please let's make sure this gets done for 10.  We're planning on
breaking a lot of config things in 10, so it would be MUCH easier on
users if we do the recovery.conf changes in that version as well.

-- 
--
Josh Berkus
Red Hat OSAS
(any opinions are my own)

On 12/01/2016 05:58 PM, Magnus Hagander wrote:
>     >> * Add docs: "Guide to changes in recovery.conf in 10.0"
>     >
>     > Hmm, we don't usually write the docs in terms of how things are
>     > different from a previous version.  Might seem strange in 5 years.
>     > Not sure what's best, here.
> 
>     A good chunk in the release notes would make sense as well?
> 
> 
> It would.
> 
> And in fairness, having such a "guide to changes" chapter in each
> release probably *would* be a good idea. But it would take resources to
> make that. The release notes are good, but having a more hand-holding
> version explaining incompatible changes in "regular sentences" would
> probably be quite useful to users.

We will have enough major changes in 10.0 to warrant writing one of
these.  Maybe not as part of the official docs, but as a set of wiki
pages or similar.

-- 
--
Josh Berkus
Red Hat OSAS
(any opinions are my own)

On 12/01/2016 05:58 PM, Magnus Hagander wrote:
>     >> * Add docs: "Guide to changes in recovery.conf in 10.0"
>     >
>     > Hmm, we don't usually write the docs in terms of how things are
>     > different from a previous version.  Might seem strange in 5 years.
>     > Not sure what's best, here.
> 
>     A good chunk in the release notes would make sense as well?
> 
> 
> It would.
> 
> And in fairness, having such a "guide to changes" chapter in each
> release probably *would* be a good idea. But it would take resources to
> make that. The release notes are good, but having a more hand-holding
> version explaining incompatible changes in "regular sentences" would
> probably be quite useful to users.

We will have enough major changes in 10.0 to warrant writing one of
these.  Maybe not as part of the official docs, but as a set of wiki
pages or similar.

-- 
--
Josh Berkus
Red Hat OSAS
(any opinions are my own)

Josh Berkus <josh@agliodbs.com> writes:
> On 12/01/2016 05:58 PM, Magnus Hagander wrote:
>> And in fairness, having such a "guide to changes" chapter in each
>> release probably *would* be a good idea. But it would take resources to
>> make that. The release notes are good, but having a more hand-holding
>> version explaining incompatible changes in "regular sentences" would
>> probably be quite useful to users.

> We will have enough major changes in 10.0 to warrant writing one of
> these.  Maybe not as part of the official docs, but as a set of wiki
> pages or similar.

Seems to me this is exactly the release notes' turf.  If you think the
release notes aren't clear enough, step right up and help improve them.

My own take on it is that the release notes are already a massive
amount of work, and putting duplicative material in a bunch of other
places isn't going to make things better, it'll just increase the
maintenance burden.
        regards, tom lane

On 12/08/2016 04:16 PM, Tom Lane wrote:
> Josh Berkus <josh@agliodbs.com> writes:
>> On 12/01/2016 05:58 PM, Magnus Hagander wrote:
>>> And in fairness, having such a "guide to changes" chapter in each
>>> release probably *would* be a good idea. But it would take resources to
>>> make that. The release notes are good, but having a more hand-holding
>>> version explaining incompatible changes in "regular sentences" would
>>> probably be quite useful to users.
> 
>> We will have enough major changes in 10.0 to warrant writing one of
>> these.  Maybe not as part of the official docs, but as a set of wiki
>> pages or similar.
> 
> Seems to me this is exactly the release notes' turf.  If you think the
> release notes aren't clear enough, step right up and help improve them.
> 
> My own take on it is that the release notes are already a massive
> amount of work, and putting duplicative material in a bunch of other
> places isn't going to make things better, it'll just increase the
> maintenance burden.

This would mean adding literally pages of material to the release notes.
In the past, folks have been very negative on anything which would make
the release notes longer.  Are you sure?

-- 
--
Josh Berkus
Red Hat OSAS
(any opinions are my own)

On Fri, Dec 9, 2016 at 9:34 AM, Josh Berkus <josh@agliodbs.com> wrote:
> On 12/08/2016 04:16 PM, Tom Lane wrote:
>> Josh Berkus <josh@agliodbs.com> writes:
>>> On 12/01/2016 05:58 PM, Magnus Hagander wrote:
>>>> And in fairness, having such a "guide to changes" chapter in each
>>>> release probably *would* be a good idea. But it would take resources to
>>>> make that. The release notes are good, but having a more hand-holding
>>>> version explaining incompatible changes in "regular sentences" would
>>>> probably be quite useful to users.
>>
>>> We will have enough major changes in 10.0 to warrant writing one of
>>> these.  Maybe not as part of the official docs, but as a set of wiki
>>> pages or similar.
>>
>> Seems to me this is exactly the release notes' turf.  If you think the
>> release notes aren't clear enough, step right up and help improve them.
>>
>> My own take on it is that the release notes are already a massive
>> amount of work, and putting duplicative material in a bunch of other
>> places isn't going to make things better, it'll just increase the
>> maintenance burden.
>
> This would mean adding literally pages of material to the release notes.
> In the past, folks have been very negative on anything which would make
> the release notes longer.  Are you sure?

As that's a per-version information, that seems adapted to me. There
could be as well in the release notes a link to the portion of the
docs holding this manual. Definitely this should be self-contained in
the docs, and not mention the wiki. My 2c.
-- 
Michael

On Fri, Dec  9, 2016 at 09:46:44AM +0900, Michael Paquier wrote:
> >> My own take on it is that the release notes are already a massive
> >> amount of work, and putting duplicative material in a bunch of other
> >> places isn't going to make things better, it'll just increase the
> >> maintenance burden.
> >
> > This would mean adding literally pages of material to the release notes.
> > In the past, folks have been very negative on anything which would make
> > the release notes longer.  Are you sure?
> 
> As that's a per-version information, that seems adapted to me. There
> could be as well in the release notes a link to the portion of the
> docs holding this manual. Definitely this should be self-contained in
> the docs, and not mention the wiki. My 2c.

Yes, that is the usual approach.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

On 12/14/2016 08:06 AM, Bruce Momjian wrote:
> On Fri, Dec  9, 2016 at 09:46:44AM +0900, Michael Paquier wrote:
>>>> My own take on it is that the release notes are already a massive
>>>> amount of work, and putting duplicative material in a bunch of other
>>>> places isn't going to make things better, it'll just increase the
>>>> maintenance burden.
>>>
>>> This would mean adding literally pages of material to the release notes.
>>> In the past, folks have been very negative on anything which would make
>>> the release notes longer.  Are you sure?
>>
>> As that's a per-version information, that seems adapted to me. There
>> could be as well in the release notes a link to the portion of the
>> docs holding this manual. Definitely this should be self-contained in
>> the docs, and not mention the wiki. My 2c.
> 
> Yes, that is the usual approach.
> 

So where in the docs should these go, then?  We don't (currently) have a
place for this kind of doc.  Appendices?

-- 
--
Josh Berkus
Red Hat OSAS
(any opinions are my own)

On Wed, Dec 14, 2016 at 02:29:07PM -0800, Josh Berkus wrote:
> On 12/14/2016 08:06 AM, Bruce Momjian wrote:
> > On Fri, Dec  9, 2016 at 09:46:44AM +0900, Michael Paquier wrote:
> >>>> My own take on it is that the release notes are already a massive
> >>>> amount of work, and putting duplicative material in a bunch of other
> >>>> places isn't going to make things better, it'll just increase the
> >>>> maintenance burden.
> >>>
> >>> This would mean adding literally pages of material to the release notes.
> >>> In the past, folks have been very negative on anything which would make
> >>> the release notes longer.  Are you sure?
> >>
> >> As that's a per-version information, that seems adapted to me. There
> >> could be as well in the release notes a link to the portion of the
> >> docs holding this manual. Definitely this should be self-contained in
> >> the docs, and not mention the wiki. My 2c.
> > 
> > Yes, that is the usual approach.
> > 
> 
> So where in the docs should these go, then?  We don't (currently) have a
> place for this kind of doc.  Appendices?

You are saying this is more massive than any other change we have made
in the past?  In general, what need to be documented?

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

Magnus Hagander <magnus@hagander.net> writes:
> On Thu, Dec 15, 2016 at 1:11 AM, Bruce Momjian <bruce@momjian.us> wrote:
>> You are saying this is more massive than any other change we have made
>> in the past?  In general, what need to be documented?

> I don't necessarily think it's because it's more massive than any chance we
> have made before. I think it's more that this is something that we probably
> should've had before, and just didn't.

> Right now we basically have a bulletpoint list of things that are new, with
> a section about things that are incompatible.  Having an actual section
> with more detailed descriptions of how to handle these changes would
> definitely be a win. it shouldn't *just* be for these changes of course, it
> should be for any other changes that are large enough to benefit from more
> than a oneliner about the fact that they've changed.

Yeah, it seems to me that where this is ending up is "we may need to
write more in the compatibility entries than we have in the past".
I don't see any problem with that, particularly if someone other than
Bruce or me is volunteering to write it ;-)
        regards, tom lane

On 12/15/2016 12:54 PM, Tom Lane wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> On Thu, Dec 15, 2016 at 1:11 AM, Bruce Momjian <bruce@momjian.us> wrote:
>>> You are saying this is more massive than any other change we have made
>>> in the past?  In general, what need to be documented?
> 
>> I don't necessarily think it's because it's more massive than any chance we
>> have made before. I think it's more that this is something that we probably
>> should've had before, and just didn't.
> 
>> Right now we basically have a bulletpoint list of things that are new, with
>> a section about things that are incompatible.  Having an actual section
>> with more detailed descriptions of how to handle these changes would
>> definitely be a win. it shouldn't *just* be for these changes of course, it
>> should be for any other changes that are large enough to benefit from more
>> than a oneliner about the fact that they've changed.
> 
> Yeah, it seems to me that where this is ending up is "we may need to
> write more in the compatibility entries than we have in the past".
> I don't see any problem with that, particularly if someone other than
> Bruce or me is volunteering to write it ;-)

I'm up for writing it (with help from feature owners), provided that I
don't have to spend time arguing that it's not too long, or that I
should put it somewhere different.  So can we settle the "where"
question first?

-- 
--
Josh Berkus
Red Hat OSAS
(any opinions are my own)

On Thu, Dec 15, 2016 at 04:16:36PM -0800, Josh Berkus wrote:
> On 12/15/2016 12:54 PM, Tom Lane wrote:
> > Magnus Hagander <magnus@hagander.net> writes:
> >> On Thu, Dec 15, 2016 at 1:11 AM, Bruce Momjian <bruce@momjian.us> wrote:
> >>> You are saying this is more massive than any other change we have made
> >>> in the past?  In general, what need to be documented?
> > 
> >> I don't necessarily think it's because it's more massive than any chance we
> >> have made before. I think it's more that this is something that we probably
> >> should've had before, and just didn't.
> > 
> >> Right now we basically have a bulletpoint list of things that are new, with
> >> a section about things that are incompatible.  Having an actual section
> >> with more detailed descriptions of how to handle these changes would
> >> definitely be a win. it shouldn't *just* be for these changes of course, it
> >> should be for any other changes that are large enough to benefit from more
> >> than a oneliner about the fact that they've changed.
> > 
> > Yeah, it seems to me that where this is ending up is "we may need to
> > write more in the compatibility entries than we have in the past".
> > I don't see any problem with that, particularly if someone other than
> > Bruce or me is volunteering to write it ;-)
> 
> I'm up for writing it (with help from feature owners), provided that I
> don't have to spend time arguing that it's not too long, or that I
> should put it somewhere different.  So can we settle the "where"
> question first?

I am fine with the release note, or the release notes plus a link to a
wiki, like we have done in the past with complex fixes in minor
releases:
https://wiki.postgresql.org/wiki/20110408pg_upgrade_fix

I think what we _don't_ want is the information _only_ in the wiki, nor
do we want to carry around migration instructions in our docs forever.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

On Fri, Dec 16, 2016 at 5:29 PM, Bruce Momjian <bruce@momjian.us> wrote:
> I am fine with the release note, or the release notes plus a link to a
> wiki, like we have done in the past with complex fixes in minor
> releases:
>
>         https://wiki.postgresql.org/wiki/20110408pg_upgrade_fix
>
> I think what we _don't_ want is the information _only_ in the wiki, nor
> do we want to carry around migration instructions in our docs forever.

I really don't see why we're resisting Josh's idea of putting a more
complex set of migration instructions in the documentation someplace.
Seems useful to me.  Sure, we'd have to "carry" it forever, but we
could make a policy of removing migration instructions for releases
that are now EOL.  And even if we didn't, it's not like extra
documentation comes with some big cost.  I think a lot of users would
benefit from a substantial expansion of our documentation, not just in
this area but in many other areas as well.  I bet that we could double
the size of the documentation and users would love it; the hard part
would be finding qualified people to write high-quality documentation
of all the things that aren't well-explained in the current docs.

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

On Fri, Dec 16, 2016 at 07:19:43PM -0500, Robert Haas wrote:
> On Fri, Dec 16, 2016 at 5:29 PM, Bruce Momjian <bruce@momjian.us> wrote:
> > I am fine with the release note, or the release notes plus a link to a
> > wiki, like we have done in the past with complex fixes in minor
> > releases:
> >
> >         https://wiki.postgresql.org/wiki/20110408pg_upgrade_fix
> >
> > I think what we _don't_ want is the information _only_ in the wiki, nor
> > do we want to carry around migration instructions in our docs forever.
> 
> I really don't see why we're resisting Josh's idea of putting a more
> complex set of migration instructions in the documentation someplace.
> Seems useful to me.  Sure, we'd have to "carry" it forever, but we
> could make a policy of removing migration instructions for releases
> that are now EOL.  And even if we didn't, it's not like extra
> documentation comes with some big cost.  I think a lot of users would
> benefit from a substantial expansion of our documentation, not just in
> this area but in many other areas as well.  I bet that we could double
> the size of the documentation and users would love it; the hard part
> would be finding qualified people to write high-quality documentation
> of all the things that aren't well-explained in the current docs.

Well, the item has a limited useful lifespan, and we could adjust the
wiki page between minor releases if we found problems/improvements.  I
suppose if we create a "Migration" section in the documentation it would
be harmless enough, but it is unclear what would be in there and what
would be in the release notes.  What would be the title of the
subsection for this?   "Migrating recovery.conf to Postgres 10?"  Maybe
make it a subsection of the release notes in the docs?

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

Bruce Momjian <bruce@momjian.us> writes:
> On Fri, Dec 16, 2016 at 07:19:43PM -0500, Robert Haas wrote:
>> I really don't see why we're resisting Josh's idea of putting a more
>> complex set of migration instructions in the documentation someplace.
>> Seems useful to me.  Sure, we'd have to "carry" it forever, but we
>> could make a policy of removing migration instructions for releases
>> that are now EOL.

> Well, the item has a limited useful lifespan, and we could adjust the
> wiki page between minor releases if we found problems/improvements.  I
> suppose if we create a "Migration" section in the documentation it would
> be harmless enough, but it is unclear what would be in there and what
> would be in the release notes.  What would be the title of the
> subsection for this?   "Migrating recovery.conf to Postgres 10?"  Maybe
> make it a subsection of the release notes in the docs?

I've been thinking for awhile that we need to start retiring
ancient-branch release notes from the active documentation.  I got
pushback on that when I proposed it to the list (too lazy to look up
the thread right now).  But it would certainly be an easier sell
to make the release notes more voluminous if we started cutting off
the long tail of ancient notes.

I'm still not seeing any value in putting this sort of info into
a documentation section that's distinct from the release notes.
We've used links to wiki pages in the past when the information
seemed to be in flux, and that's reasonable.  But what's the point
of just linking to somewhere else in the same document?
        regards, tom lane

On Fri, Dec 16, 2016 at 8:07 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I'm still not seeing any value in putting this sort of info into
> a documentation section that's distinct from the release notes.
> We've used links to wiki pages in the past when the information
> seemed to be in flux, and that's reasonable.  But what's the point
> of just linking to somewhere else in the same document?

If the explanation is just a few sentences long, I see no reason not
to include it in the release notes.  But if it's comparable in length
to a moderately-long chapter then why would we not make it its own
chapter?  I think your argument boils down to "people probably don't
need very much detail about this".  But I think that's the wrong line
of thinking.  In my view, we ought to ship just about as much quality
documentation as people are willing to write.  Saying that we're going
to reject good-quality documentation because we don't want to have too
much of it is like saying we want to reject good-quality features
because we don't want to have too many of them, or good-quality
regression tests because we don't want to have too much code coverage,
or good-quality bug fixes because we don't want to have too few bugs.
It is of course true that all of these things can be done in a bad way
or to excess: our documentation could be a terabyte!  our features
could be so numerous as to befuddle even expert users!  our regression
tests could run for a hundred hours on a supercomputer!  we could fix
so many basically-trivial bugs that our committers all fall sobbing to
the floor with crippling repetitive strain injuries!  But let's not
FUD things that are basically positive.  I think it's fantastic that
Josh Berkus wants to write more documentation, and if 10 other people
show up to do similar work, I'll buy them all a beer at the next
conference we're at together.  I almost wrote "I'll buy them all a
bear" but that might be over the top.

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

On Fri, Dec 16, 2016 at 08:52:25PM -0500, Robert Haas wrote:
> On Fri, Dec 16, 2016 at 8:07 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > I'm still not seeing any value in putting this sort of info into
> > a documentation section that's distinct from the release notes.
> > We've used links to wiki pages in the past when the information
> > seemed to be in flux, and that's reasonable.  But what's the point
> > of just linking to somewhere else in the same document?
> 
> If the explanation is just a few sentences long, I see no reason not
> to include it in the release notes.  But if it's comparable in length
> to a moderately-long chapter then why would we not make it its own
> chapter?  I think your argument boils down to "people probably don't
> need very much detail about this".  But I think that's the wrong line
> of thinking.  In my view, we ought to ship just about as much quality
> documentation as people are willing to write.  Saying that we're going
> to reject good-quality documentation because we don't want to have too
> much of it is like saying we want to reject good-quality features
> because we don't want to have too many of them, or good-quality
> regression tests because we don't want to have too much code coverage,

[ I stopped reading after this. ]

The point is that the documentation about the recovery.conf changes in
Postgres are only interesting to people migrating to Postgres 10, i.e.
this is not quality documentation for someone going from Postgres 10 to
Postgres 11.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

On Sat, Dec 17, 2016 at 8:52 AM, Magnus Hagander <magnus@hagander.net> wrote:
> On Sat, Dec 17, 2016 at 5:42 AM, Bruce Momjian <bruce@momjian.us> wrote:
>> On Fri, Dec 16, 2016 at 08:52:25PM -0500, Robert Haas wrote:
>> > On Fri, Dec 16, 2016 at 8:07 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> > > I'm still not seeing any value in putting this sort of info into
>> > > a documentation section that's distinct from the release notes.
>> > > We've used links to wiki pages in the past when the information
>> > > seemed to be in flux, and that's reasonable.  But what's the point
>> > > of just linking to somewhere else in the same document?
>> >
>> > If the explanation is just a few sentences long, I see no reason not
>> > to include it in the release notes.  But if it's comparable in length
>> > to a moderately-long chapter then why would we not make it its own
>> > chapter?  I think your argument boils down to "people probably don't
>> > need very much detail about this".  But I think that's the wrong line
>> > of thinking.  In my view, we ought to ship just about as much quality
>> > documentation as people are willing to write.  Saying that we're going
>> > to reject good-quality documentation because we don't want to have too
>> > much of it is like saying we want to reject good-quality features
>> > because we don't want to have too many of them, or good-quality
>> > regression tests because we don't want to have too much code coverage,
>>
>> [ I stopped reading after this. ]
>>
>> The point is that the documentation about the recovery.conf changes in
>> Postgres are only interesting to people migrating to Postgres 10, i.e.
>> this is not quality documentation for someone going from Postgres 10 to
>> Postgres 11.
>>
>
> They will also be interesting to people going from 9.4 to 11, or from 9.3 to
> 12. Or from 9.5 to 13.
>
> They only become uninteresting when we stop supporting 9.6 which is the last
> version that didn't have that functionality.

Right, exactly.  Also, even if it were true that they were only
interesting to people migrating to version 10, that doesn't mean we
shouldn't have them.

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

On Sat, Dec 17, 2016 at 02:52:41PM +0100, Magnus Hagander wrote:
>     The point is that the documentation about the recovery.conf changes in
>     Postgres are only interesting to people migrating to Postgres 10, i.e.
>     this is not quality documentation for someone going from Postgres 10 to
>     Postgres 11.
> 
> 
> 
> They will also be interesting to people going from 9.4 to 11, or from 9.3 to
> 12. Or from 9.5 to 13.
> 
> They only become uninteresting when we stop supporting 9.6 which is the last
> version that didn't have that functionality. 

No, they become uninteresting to anyone who has passed Postgres 10.  I
would argue they are still required to be around even after we stop
supporting Postgres 9.6 because we know everyone will not upgrade off of
supported releases once we stop supporting them.  This means we can
effectively never remove the information.

Right now if you migrate from Postgres 8.0 to Postgres 9.6, all the
information you need is in the 9.6 documentation.  If you were to remove
migration details from 8.4 to 9.0 they would have to look at the 9.0
docs to get a full picture of how to migrate.

Again, I am fine putting this as a subsection of the release notes, but
let's not pretend it is some extra section we can remove in five years.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

On Sun, Dec 18, 2016 at 12:41:01PM +0100, Magnus Hagander wrote:
>     No, they become uninteresting to anyone who has passed Postgres 10.  I
>     would argue they are still required to be around even after we stop
>     supporting Postgres 9.6 because we know everyone will not upgrade off of
>     supported releases once we stop supporting them.  This means we can
>     effectively never remove the information.
> 
> 
> This is true, but I think it's also safe to say that it's acceptable that if
> you are upgrading from an unsupported version you need to read more than one
> set of documentation -- one set to get to a supported one, and one get on from
> there.

How do you propose they find that other documentation set if upgrading
from 9.6 to version 16?  Do we put the old docs somewhere on our web
site?  Do we have them download a tarball and unpack it?  How do we
handle old translated doc versions?  I realize the wiki isn't
translated, but if we have translators translate it, we should keep it
available.

>     Right now if you migrate from Postgres 8.0 to Postgres 9.6, all the
>     information you need is in the 9.6 documentation.  If you were to remove
>     migration details from 8.4 to 9.0 they would have to look at the 9.0
>     docs to get a full picture of how to migrate.
> 
> 
> In fairness, all the information you need is definitely not in the
> documentation. You have all the release notes, that is true. But for a lot of
> people and in the case of a lot of features, that is not at all enough
> information. But it's true in the sense that it's just as much information as

Uh, what exactly is this additional information you are talking about? 
Blogs?  Are you saying we have information about upgrades we have
captured but discard?

>     Again, I am fine putting this as a subsection of the release notes, but
>     let's not pretend it is some extra section we can remove in five years.
> 
> 
> Depends on what we decide to do about it, but sure, it could certainly turn
> into another section that we keep around (whether as part of the release notes,
> or as a separate "upgrade steps" section or something).

I suggest whatever we do, we place the information in a permanent
location that isn't moved or removed.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

On Sun, Dec 18, 2016 at 02:02:58PM +0100, Magnus Hagander wrote:
>     >     Again, I am fine putting this as a subsection of the release notes,
>     but
>     >     let's not pretend it is some extra section we can remove in five
>     years.
>     >
>     >
>     > Depends on what we decide to do about it, but sure, it could certainly
>     turn
>     > into another section that we keep around (whether as part of the release
>     notes,
>     > or as a separate "upgrade steps" section or something).
> 
>     I suggest whatever we do, we place the information in a permanent
>     location that isn't moved or removed.
> 
> 
> 
> +1. Absolutely. That's a very important point.

That is really my only point --- wherever you put it, expect it to live
there forever.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

On Sun, Dec 18, 2016 at 02:14:06PM +0100, Magnus Hagander wrote:
>     >     turn
>     >     > into another section that we keep around (whether as part of the
>     release
>     >     notes,
>     >     > or as a separate "upgrade steps" section or something).
>     >
>     >     I suggest whatever we do, we place the information in a permanent
>     >     location that isn't moved or removed.
>     >
>     >
>     >
>     > +1. Absolutely. That's a very important point.
> 
>     That is really my only point --- wherever you put it, expect it to live
>     there forever.
>
> Then in the end, it turns out we're in strong agreement :) 

Yes, but there was talk of adding it to the docs, then removing it in
later releases --- that's what I think is a bad idea.  I think it would
be logical to have a sub-document underneath each major release note
section with more detailed instructions.  This could be done for minor
releases as well where necessary.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

On 12/16/16 8:52 PM, Robert Haas wrote:
> If the explanation is just a few sentences long, I see no reason not
> to include it in the release notes.

As far as I can tell from the latest posted patch, the upgrade
instructions are

- To trigger recovery, create an empty file recovery.trigger instead of
recovery.conf.

- All parameters formerly in recovery.conf are now regular
postgresql.conf parameters.  For backward compatibility, recovery.conf
is read after postgresql.conf, but the parameters can now be put into
postgresql.conf if desired.

Some of that might be subject to patch review, but it's probably not
going to be much longer than that.  So I think that will fit well into
the usual release notes section.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 19 December 2016 at 21:29, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 12/16/16 8:52 PM, Robert Haas wrote:
>> If the explanation is just a few sentences long, I see no reason not
>> to include it in the release notes.
>
> As far as I can tell from the latest posted patch, the upgrade
> instructions are
>
> - To trigger recovery, create an empty file recovery.trigger instead of
> recovery.conf.
>
> - All parameters formerly in recovery.conf are now regular
> postgresql.conf parameters.  For backward compatibility, recovery.conf
> is read after postgresql.conf, but the parameters can now be put into
> postgresql.conf if desired.
>
> Some of that might be subject to patch review, but it's probably not
> going to be much longer than that.  So I think that will fit well into
> the usual release notes section.

Although that's right, there is more detail.

The current list is this... based upon discussion in Tokyo dev meeting

* Any use of the earlier recovery.conf or any of the old recovery
parameter names causes an ERROR, so we have a clear API break

* To trigger recovery, create an empty recovery.trigger file. Same as
recovery.conf with standby_mode = off

* To trigger standby, create an empty standby.trigger file. Same as
recovery.conf with standby_mode = on

* Trigger files can be placed in a writable directory outside of the
data directory, trigger_directory
(I would also like to rename the concept of "trigger file" to another
word/phrase not already used for something else, c.f. table triggers)

* recovery.conf parameters are all moved to postgresql.conf, with these changes
a) standby_mode no longer exists
b) trigger_file is replaced by promote_trigger_file
equivalent parameters will be added for c) recovery_trigger_file and
d) standby_trigger_file
e) recovery_target is a new parameter with the two part content
mentioned upthread, e.g. 'xid 1234'
f) recovery_target and all trigger_file parameters are now reloadable,
not server start
g) recovery parameters are shown in the postgresql.conf.sample, but
commented out
h) recovery.conf.sample is removed
which leaves many of the parameters with same name and meaning as
before, but enough change to be a clear API break that needs people to
understand the changes

* pg_basebackup -R will generate a parameter file written to data
directory that will allow it to work, even when postgresql.conf is in
a different directory
(this bit was the most awkward part of the list of changes)

* hot_standby parameter would be removed

I think I can work this into the docs without anything worth further
discussion, but let me write that first.

The release notes can warn about the old API generating an ERROR, with
a multiple links to discussion of how it now works.

Merry Christmas everybody, new patch in time for New Year, further
discussion welcome

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

On Tue, Dec 20, 2016 at 7:11 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
> On 19 December 2016 at 21:29, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>> On 12/16/16 8:52 PM, Robert Haas wrote:
>>> If the explanation is just a few sentences long, I see no reason not
>>> to include it in the release notes.
>>
>> As far as I can tell from the latest posted patch, the upgrade
>> instructions are
>>
>> - To trigger recovery, create an empty file recovery.trigger instead of
>> recovery.conf.
>>
>> - All parameters formerly in recovery.conf are now regular
>> postgresql.conf parameters.  For backward compatibility, recovery.conf
>> is read after postgresql.conf, but the parameters can now be put into
>> postgresql.conf if desired.
>>
>> Some of that might be subject to patch review, but it's probably not
>> going to be much longer than that.  So I think that will fit well into
>> the usual release notes section.
>
> Although that's right, there is more detail.
>
> The current list is this... based upon discussion in Tokyo dev meeting
>
> * Any use of the earlier recovery.conf or any of the old recovery
> parameter names causes an ERROR, so we have a clear API break
>
> * To trigger recovery, create an empty recovery.trigger file. Same as
> recovery.conf with standby_mode = off
>
> * To trigger standby, create an empty standby.trigger file. Same as
> recovery.conf with standby_mode = on

API for crash recovery will never be changed. That is, crash recovery needs
neither recovery.trigger nor standby.trigger. When the server starts a crash
recovery without any trigger file, any recovery parameter settings in
postgresql.conf are ignored. Right?

Regards,

-- 
Fujii Masao

On 20 December 2016 at 15:03, Fujii Masao <masao.fujii@gmail.com> wrote:

> API for crash recovery will never be changed. That is, crash recovery needs
> neither recovery.trigger nor standby.trigger. When the server starts a crash
> recovery without any trigger file, any recovery parameter settings in
> postgresql.conf are ignored. Right?

Yes. There are no conceptual changes, just the API.

The goals are: visibility and reloading of recovery parameters,
removal of special case code.

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

On 12/19/2016 01:29 PM, Peter Eisentraut wrote:
> On 12/16/16 8:52 PM, Robert Haas wrote:
>> > If the explanation is just a few sentences long, I see no reason not
>> > to include it in the release notes.
> As far as I can tell from the latest posted patch, the upgrade
> instructions are
>
> - To trigger recovery, create an empty file recovery.trigger instead of
> recovery.conf.
>
> - All parameters formerly in recovery.conf are now regular
> postgresql.conf parameters.  For backward compatibility, recovery.conf
> is read after postgresql.conf, but the parameters can now be put into
> postgresql.conf if desired.

Aren't we changing how some of the parameters work?

>
> Some of that might be subject to patch review, but it's probably not
> going to be much longer than that.  So I think that will fit well into
> the usual release notes section.

Changed the subject line of this thread because people are becoming
confused about the new topic.

I'm not talking about *just* the recovery.conf changes.  We're making a
lot of changes to 10 which will require user action, and there may be
more before 10 is baked.  For example, dealing with the version
numbering change.  I started a list of the things we're breaking for 10,
but I don't have it with me at the moment.  There's more than 3 things
on it.

And then there's docs for stuff which isn't *required* by upgrading, but
would be a good idea.  For example, we'll eventually want a doc on how
to migrate old-style partitioned tables to new-style partitioned tables.

In any case, Peter's response shows *exactly* why I don't want to put
this documentation into the release notes: because people are going to
complain it's too long and want to truncate it. Writing the docs will be
hard enough; if I (or anyone else) has to argue about whether or not
they're too long, I'm just going to drop the patch and walk away.

--
--
Josh Berkus
Red Hat OSAS
(any opinions are my own)

On 20 December 2016 at 15:11, Simon Riggs <simon@2ndquadrant.com> wrote:
> On 20 December 2016 at 15:03, Fujii Masao <masao.fujii@gmail.com> wrote:
>
>> API for crash recovery will never be changed. That is, crash recovery needs
>> neither recovery.trigger nor standby.trigger. When the server starts a crash
>> recovery without any trigger file, any recovery parameter settings in
>> postgresql.conf are ignored. Right?
>
> Yes. There are no conceptual changes, just the API.
>
> The goals are: visibility and reloading of recovery parameters,
> removal of special case code.

OK, so here's the patch, plus doc cleanup patch.

Trying to fit recovery targets into one parameter was really not
feasible, though I tried. Michael's suggested approach of using
recovery_target_type and recovery_target_value has been more
practical, bearing in mind the 3 other relevant parameters.


1. Any use of the earlier recovery.conf or any of the old recovery
parameter names causes an ERROR, so we have a clear API break

2. Signal files can be placed in a writable directory outside of the
data directory, signal_file_directory

3. To signal recovery, create an empty recovery.signal file. Same as
recovery.conf with standby_mode = off

4. To signal standby, create an empty standby.signal file. Same as
recovery.conf with standby_mode = on

5. recovery.conf parameters are all moved to postgresql.conf, with these changes
a) standby_mode no longer exists
b) trigger_file no longer exists… create a file in signal_directory
c)
recovery_target_type
recovery_target_value
are two new parameters that specify the most important type and value of
targeted recovery, though there are other parameters that affect the
outcome also.

e.g.
recovery_target_type = ‘xid’
recovery_target_value = ‘1234’

d) recovery target are now reloadable, not server start only

e) recovery parameters are shown in the postgresql.conf.sample, but
commented out

f) primary_conninfo and primary_slotinfo are renamed to
sender_conninfo and sender_slotinfo to more accurately reflect that these
parameters are no longer primary/master only

g) Recovery Config chapter in docs is retained (for now, at least) and
will later add a link from the main docs to explain this.

I've left the following points for later discussion/cleanup

* recovery.conf.sample will be removed

* pg_basebackup -R will generate a parameter file written to data
directory that will allow it to work, even when postgresql.conf is in
a different directory
(this bit was the most awkward part of the list of changes)

* hot_standby parameter would be removed


Tests have been modified also, but regrettably at this point only 5
out of 8 regression tests pass. This is most likely because I've had
to make more complex changes around timeline handling. I'll post again
tomorrow when I fix them.

Next steps I plan for this release

* Relocate some more code out of xlog.c and guc.c

* Full rewrite of main text docs

* pg_ctl demote
Allows us to avoid incrementing the timeline when performing a switchover

--
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 Sun, Jan 1, 2017 at 4:14 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
> Trying to fit recovery targets into one parameter was really not
> feasible, though I tried.

What was the problem?

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

On 3 January 2017 at 15:50, Robert Haas <robertmhaas@gmail.com> wrote:
> On Sun, Jan 1, 2017 at 4:14 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
>> Trying to fit recovery targets into one parameter was really not
>> feasible, though I tried.
>
> What was the problem?

There are 5 different parameters that affect the recovery target, 3 of
which are optional. That is much more complex than the two compulsory
parameters suggested when the proposal was made and in my view tips
the balance over the edge into pointlessness. Michael's suggestion for
2 parameters works well for this case.

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

On Tue, Jan 3, 2017 at 11:21 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
> On 3 January 2017 at 15:50, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Sun, Jan 1, 2017 at 4:14 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
>>> Trying to fit recovery targets into one parameter was really not
>>> feasible, though I tried.
>>
>> What was the problem?
>
> There are 5 different parameters that affect the recovery target, 3 of
> which are optional. That is much more complex than the two compulsory
> parameters suggested when the proposal was made and in my view tips
> the balance over the edge into pointlessness. Michael's suggestion for
> 2 parameters works well for this case.

I still think merging recovery_target_type and recovery_target_value
into a single parameter could make sense, never mind the other three.
But I don't really want to argue about it any more.

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

On 3 January 2017 at 16:47, Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Jan 3, 2017 at 11:21 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
>> On 3 January 2017 at 15:50, Robert Haas <robertmhaas@gmail.com> wrote:
>>> On Sun, Jan 1, 2017 at 4:14 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
>>>> Trying to fit recovery targets into one parameter was really not
>>>> feasible, though I tried.
>>>
>>> What was the problem?
>>
>> There are 5 different parameters that affect the recovery target, 3 of
>> which are optional. That is much more complex than the two compulsory
>> parameters suggested when the proposal was made and in my view tips
>> the balance over the edge into pointlessness. Michael's suggestion for
>> 2 parameters works well for this case.
>
> I still think merging recovery_target_type and recovery_target_value
> into a single parameter could make sense, never mind the other three.
> But I don't really want to argue about it any more.

We all agree that reducing number parameters made sense and would
remove weird quirks of multiple settings. When we were discussing the
idea of replacing them with just 1 parameter, that idea made sense to
me after I'd thought about it, but it would actually be 4 parameters.
I couldn't see a way to wave the corner case parameters away.

After more detailed thought I don't see the point of reducing the
number of parameters from to 4, especially if that reduction comes
with increased complexity for one of the parameters. Better to just
have 5 as Michael suggested.

This patch replaces the previous 8 parameters with just 5.

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

On 01/03/2017 08:47 AM, Robert Haas wrote:
> On Tue, Jan 3, 2017 at 11:21 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
>> On 3 January 2017 at 15:50, Robert Haas <robertmhaas@gmail.com> wrote:
>>> On Sun, Jan 1, 2017 at 4:14 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
>>>> Trying to fit recovery targets into one parameter was really not
>>>> feasible, though I tried.
>>>
>>> What was the problem?
>>
>> There are 5 different parameters that affect the recovery target, 3 of
>> which are optional. That is much more complex than the two compulsory
>> parameters suggested when the proposal was made and in my view tips
>> the balance over the edge into pointlessness. Michael's suggestion for
>> 2 parameters works well for this case.
> 
> I still think merging recovery_target_type and recovery_target_value
> into a single parameter could make sense, never mind the other three.
> But I don't really want to argue about it any more.
> 

Either solution works for me.

-- 
--
Josh Berkus
Red Hat OSAS
(any opinions are my own)

On 1/1/17 4:14 PM, Simon Riggs wrote:
> OK, so here's the patch, plus doc cleanup patch.

I don't think this patch is likely to succeed if we throw in more ideas
in every round.

I think we have or are approaching agreement on moving recovery.conf
into postgresql.conf, making the settings reloadable, and adding signal
(formerly trigger) files to trigger recovery or standby.  I think that
is a useful change, but it's already big enough and needs extensive
reviewing and testing.

All the other stuff, such as regrouping the recovery parameters,
removing the hot_standby setting, renaming the primary_* parameters,
making the directory of the signal files configurable, should be
separate patches that should be discussed separately.  I think the
arguments for these latter changes are weaker, and tactically I would
focus on getting the recovery.conf move in before thinking about all the
other ones.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Mon, Jan 2, 2017 at 6:14 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
> On 20 December 2016 at 15:11, Simon Riggs <simon@2ndquadrant.com> wrote:
>> On 20 December 2016 at 15:03, Fujii Masao <masao.fujii@gmail.com> wrote:
>>
>>> API for crash recovery will never be changed. That is, crash recovery needs
>>> neither recovery.trigger nor standby.trigger. When the server starts a crash
>>> recovery without any trigger file, any recovery parameter settings in
>>> postgresql.conf are ignored. Right?
>>
>> Yes. There are no conceptual changes, just the API.
>>
>> The goals are: visibility and reloading of recovery parameters,
>> removal of special case code.
>
> OK, so here's the patch, plus doc cleanup patch.

Thanks for the patch!

> 5. recovery.conf parameters are all moved to postgresql.conf, with these changes

In current design of the patch, when recovery parameters are misconfigured
(e.g., set recovery_target_timeline to invalid timeline id) and
the configuration file is reloaded, the startup process emits FATAL error and
the server goes down. I don't think this is fine. Basically even
misconfiguration of the parameters should not cause the server crash.
If invalid settings are supplied, I think that we just should warn them
and ignore those new settings, like current other GUC is. Thought?

-        if (PrimaryConnInfo == NULL && recoveryRestoreCommand == NULL)
+        if (SenderConnInfo == NULL && recoveryRestoreCommand == NULL)

Seems "SenderConnInfo == NULL" should be changed to "SenderConnInfo[0] == '\0'".
recoveryRestoreCommand, as well.

Regards,

-- 
Fujii Masao

On Tue, Jan 10, 2017 at 4:50 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 1/1/17 4:14 PM, Simon Riggs wrote:
>> OK, so here's the patch, plus doc cleanup patch.
>
> I don't think this patch is likely to succeed if we throw in more ideas
> in every round.
>
> I think we have or are approaching agreement on moving recovery.conf
> into postgresql.conf, making the settings reloadable, and adding signal
> (formerly trigger) files to trigger recovery or standby.  I think that
> is a useful change, but it's already big enough and needs extensive
> reviewing and testing.

Agreed. It's better to focus on the "core" part of the patch firstly.

Regards,

-- 
Fujii Masao

On 9 January 2017 at 19:50, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 1/1/17 4:14 PM, Simon Riggs wrote:
>> OK, so here's the patch, plus doc cleanup patch.
>
> I don't think this patch is likely to succeed if we throw in more ideas
> in every round.
>
> I think we have or are approaching agreement on moving recovery.conf
> into postgresql.conf, making the settings reloadable, and adding signal
> (formerly trigger) files to trigger recovery or standby.  I think that
> is a useful change, but it's already big enough and needs extensive
> reviewing and testing.
>
> All the other stuff, such as regrouping the recovery parameters,
> removing the hot_standby setting, renaming the primary_* parameters,
> making the directory of the signal files configurable, should be
> separate patches that should be discussed separately.  I think the
> arguments for these latter changes are weaker, and tactically I would
> focus on getting the recovery.conf move in before thinking about all the
> other ones.

Thanks for the review.

* Removing hot_standby setting is not included in this patch; happy to
keep that separate; very low priority

* Renaming primary_* parameters - Currently we use this config setting
even when connecting to a standby, so the parameter is confusingly
named, so 10.0 is a good chance to name it correctly. Will submit as
separate patch.

* Directory for signal files was in my understanding a primary goal of
the patch. I am happy to remove that into a later submission. That
resolves, for now, the issue with pg_basebackup -R.

The above changes are fairly minor.

The main area of "design doubt" remains the implementation of the
recovery_target parameter set. Are we happy with the user interface
choices in the patch, given the understanding that the situation was
more comple than at first thought?

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

On 11 January 2017 at 09:51, Fujii Masao <masao.fujii@gmail.com> wrote:

>> 5. recovery.conf parameters are all moved to postgresql.conf, with these changes
>
> In current design of the patch, when recovery parameters are misconfigured
> (e.g., set recovery_target_timeline to invalid timeline id) and
> the configuration file is reloaded, the startup process emits FATAL error and
> the server goes down. I don't think this is fine. Basically even
> misconfiguration of the parameters should not cause the server crash.
> If invalid settings are supplied, I think that we just should warn them
> and ignore those new settings, like current other GUC is. Thought?

Thanks for your comments.

The specification of the recovery target parameters should be different, IMHO.

If the user is performing a recovery and the target of the recovery is
incorrectly specified then it is clear that the recovery cannot
continue with an imprecisely specified target. So in my understanding
we would need to either

1) issue a WARNING and pause recovery

2) issue an ERROR (which becomes FATAL in Startup process) and exit recovery

My view would be 2) is the most useful, though I am willing to hear
other points and/or go with majority view

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

On Wed, Jan 11, 2017 at 7:36 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
> The specification of the recovery target parameters should be different, IMHO.
>
> If the user is performing a recovery and the target of the recovery is
> incorrectly specified then it is clear that the recovery cannot
> continue with an imprecisely specified target. So in my understanding
> we would need to either
>
> 1) issue a WARNING and pause recovery
>
> 2) issue an ERROR (which becomes FATAL in Startup process) and exit recovery
>
> My view would be 2) is the most useful, though I am willing to hear
> other points and/or go with majority view

I agree with that. 2) is more consistent with what is in core now.
-- 
Michael

On 1/11/17 5:27 AM, Simon Riggs wrote:
> * Renaming primary_* parameters - Currently we use this config setting
> even when connecting to a standby, so the parameter is confusingly
> named, so 10.0 is a good chance to name it correctly. Will submit as
> separate patch.

I don't subscribe to the idea that 10.0 is a better chance to change
something than any other time.

I agree that the naming has become inaccurate, but it still matches the
basic use case (one primary, one standby (heck, standby is also
inaccurate now!)), and I don't recall anyone being confused by this.
Also, it is debatable whether "sender" is better.  Yes, it's a sender,
but sending what and to whom?

> * Directory for signal files was in my understanding a primary goal of
> the patch. I am happy to remove that into a later submission. That
> resolves, for now, the issue with pg_basebackup -R.

I think the issue was that some people didn't want configuration files
in the data directory.  By removing recovery.conf we accomplish that.
Signal/trigger files are not configuration (or at least it's much easier
to argue that), so I think having them in the data directory is fine.

I'm concerned that having signal files somewhere else opens up a bunch
more edge cases that need to be considered.  For example, what if
someone puts a signal file into a temporary directory that is cleared
after a server crash and restart.  That can mess up a bunch of things.

(I think I like trigger better than signal, btw.  A signal is something
asynchronous.)

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Having already agreed to remove the two mentioned aspects, I'm just
replying to fill in some historical details.

On 11 January 2017 at 17:25, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 1/11/17 5:27 AM, Simon Riggs wrote:
>> * Renaming primary_* parameters - Currently we use this config setting
>> even when connecting to a standby, so the parameter is confusingly
>> named, so 10.0 is a good chance to name it correctly. Will submit as
>> separate patch.
>
> I don't subscribe to the idea that 10.0 is a better chance to change
> something than any other time.
>
> I agree that the naming has become inaccurate, but it still matches the
> basic use case (one primary, one standby (heck, standby is also
> inaccurate now!)), and I don't recall anyone being confused by this.
> Also, it is debatable whether "sender" is better.  Yes, it's a sender,
> but sending what and to whom?

Sending server is the term already used to describe a server that is
either a master or a relaying standby.

But as already requested, I will remove from patch.

>> * Directory for signal files was in my understanding a primary goal of
>> the patch. I am happy to remove that into a later submission. That
>> resolves, for now, the issue with pg_basebackup -R.
>
> I think the issue was that some people didn't want configuration files
> in the data directory.  By removing recovery.conf we accomplish that.
> Signal/trigger files are not configuration (or at least it's much easier
> to argue that), so I think having them in the data directory is fine.

There were a considerable number of people that pushed to make the
data directory non-user writable, which is where the signal directory
came from.

I can see the argument, but since those that spoke previously have
evaporated, I'm easy.

> I'm concerned that having signal files somewhere else opens up a bunch
> more edge cases that need to be considered.  For example, what if
> someone puts a signal file into a temporary directory that is cleared
> after a server crash and restart.  That can mess up a bunch of things.

We already have trigger_file as a way to specify an alternate
directory, so if I remove signal_file_directory I will need to replace
trigger_file as a parameter.

> (I think I like trigger better than signal, btw.  A signal is something
> asynchronous.)

The code already calls them signal files, its just called trigger externally.

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

On 1/11/17 12:53 PM, Simon Riggs wrote:
>> I'm concerned that having signal files somewhere else opens up a bunch
>> more edge cases that need to be considered.  For example, what if
>> someone puts a signal file into a temporary directory that is cleared
>> after a server crash and restart.  That can mess up a bunch of things.
> 
> We already have trigger_file as a way to specify an alternate
> directory, so if I remove signal_file_directory I will need to replace
> trigger_file as a parameter.

The trigger file is for promotion, which is just a momentary nudge to
the server.  The recovery signal file or whatever we end up calling it
is long-term state, because if it disappears and the server restarts, it
will do something different.  So they are different.

I'm not strictly opposed to making the name or directory configurable,
but I'm predicting it will raise more questions than we might want to
deal with.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Jan 11, 2017 at 7:36 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
> On 11 January 2017 at 09:51, Fujii Masao <masao.fujii@gmail.com> wrote:
>
>>> 5. recovery.conf parameters are all moved to postgresql.conf, with these changes
>>
>> In current design of the patch, when recovery parameters are misconfigured
>> (e.g., set recovery_target_timeline to invalid timeline id) and
>> the configuration file is reloaded, the startup process emits FATAL error and
>> the server goes down. I don't think this is fine. Basically even
>> misconfiguration of the parameters should not cause the server crash.
>> If invalid settings are supplied, I think that we just should warn them
>> and ignore those new settings, like current other GUC is. Thought?
>
> Thanks for your comments.
>
> The specification of the recovery target parameters should be different, IMHO.
>
> If the user is performing a recovery and the target of the recovery is
> incorrectly specified then it is clear that the recovery cannot
> continue with an imprecisely specified target.

Could you tell me what case of "the target of the recovery is incorrectly
specified" are you thinking of? For example, you're thinking the case where
invalid format of timestamp value is specified in recovery_target_time?
In this case, I think that we should issue a WARNING and ignore that invalid
setting when the configuration file is reloaded. Of course, if it's the server
startup time, such invalid setting should prevent the server from starting up.

Regarding recovery_target_timeline, isn't it better to mark that parameter
as PGC_POSTMASTER? I'm not sure if we really want to change the recovery
target timeline without server restart and also not sure if that operation is
safe.

Regards,

-- 
Fujii Masao

On 12 January 2017 at 05:49, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Wed, Jan 11, 2017 at 7:36 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
>> On 11 January 2017 at 09:51, Fujii Masao <masao.fujii@gmail.com> wrote:
>>
>>>> 5. recovery.conf parameters are all moved to postgresql.conf, with these changes
>>>
>>> In current design of the patch, when recovery parameters are misconfigured
>>> (e.g., set recovery_target_timeline to invalid timeline id) and
>>> the configuration file is reloaded, the startup process emits FATAL error and
>>> the server goes down. I don't think this is fine. Basically even
>>> misconfiguration of the parameters should not cause the server crash.
>>> If invalid settings are supplied, I think that we just should warn them
>>> and ignore those new settings, like current other GUC is. Thought?
>>
>> Thanks for your comments.
>>
>> The specification of the recovery target parameters should be different, IMHO.
>>
>> If the user is performing a recovery and the target of the recovery is
>> incorrectly specified then it is clear that the recovery cannot
>> continue with an imprecisely specified target.
>
> Could you tell me what case of "the target of the recovery is incorrectly
> specified" are you thinking of? For example, you're thinking the case where
> invalid format of timestamp value is specified in recovery_target_time?
> In this case, I think that we should issue a WARNING and ignore that invalid
> setting when the configuration file is reloaded. Of course, if it's the server
> startup time, such invalid setting should prevent the server from starting up.
>
> Regarding recovery_target_timeline, isn't it better to mark that parameter
> as PGC_POSTMASTER? I'm not sure if we really want to change the recovery
> target timeline without server restart and also not sure if that operation is
> safe.

It's one of the main objectives of the patch to allow user to reset
parameters online so I don't think we should set PGC_POSTMASTER.

Of course, if the user provides an incorrect parameter value for
search, we have no way to understand their intentions. But we do know
they wanted to reset the parameter, so we should not continue using
the old parameter.

I understand your wish to throw a WARNING and "stay up", but in the
context of a recovery the whole purpose of starting the server is to
recover. If we are unable to do that because the user has failed to
set the parameter correctly then we should throw ERROR.

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

On 1/11/17 5:27 AM, Simon Riggs wrote:
> The main area of "design doubt" remains the implementation of the
> recovery_target parameter set. Are we happy with the user interface
> choices in the patch, given the understanding that the situation was
> more comple than at first thought?

Could you summarize the current proposal(s)?

Personally, I don't immediately see the need to change anything from the
parameter names that I currently see in recovery.conf.sample.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, Jan 12, 2017 at 6:46 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
> On 12 January 2017 at 05:49, Fujii Masao <masao.fujii@gmail.com> wrote:
>> On Wed, Jan 11, 2017 at 7:36 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
>>> On 11 January 2017 at 09:51, Fujii Masao <masao.fujii@gmail.com> wrote:
>>>
>>>>> 5. recovery.conf parameters are all moved to postgresql.conf, with these changes
>>>>
>>>> In current design of the patch, when recovery parameters are misconfigured
>>>> (e.g., set recovery_target_timeline to invalid timeline id) and
>>>> the configuration file is reloaded, the startup process emits FATAL error and
>>>> the server goes down. I don't think this is fine. Basically even
>>>> misconfiguration of the parameters should not cause the server crash.
>>>> If invalid settings are supplied, I think that we just should warn them
>>>> and ignore those new settings, like current other GUC is. Thought?
>>>
>>> Thanks for your comments.
>>>
>>> The specification of the recovery target parameters should be different, IMHO.
>>>
>>> If the user is performing a recovery and the target of the recovery is
>>> incorrectly specified then it is clear that the recovery cannot
>>> continue with an imprecisely specified target.
>>
>> Could you tell me what case of "the target of the recovery is incorrectly
>> specified" are you thinking of? For example, you're thinking the case where
>> invalid format of timestamp value is specified in recovery_target_time?
>> In this case, I think that we should issue a WARNING and ignore that invalid
>> setting when the configuration file is reloaded. Of course, if it's the server
>> startup time, such invalid setting should prevent the server from starting up.
>>
>> Regarding recovery_target_timeline, isn't it better to mark that parameter
>> as PGC_POSTMASTER? I'm not sure if we really want to change the recovery
>> target timeline without server restart and also not sure if that operation is
>> safe.
>
> It's one of the main objectives of the patch to allow user to reset
> parameters online so I don't think we should set PGC_POSTMASTER.

I'm OK to mark the recovery target parameters *except* recovery_target_timeline
as PGC_SIGHUP. But, as I told upthread, I'm wondering whether there is really
the use case where the target timeline needs to be changed online. Also I'm
wondering whether that online change of target timeline is actually safe.

Regards,

-- 
Fujii Masao

On 12 January 2017 at 13:34, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 1/11/17 5:27 AM, Simon Riggs wrote:
>> The main area of "design doubt" remains the implementation of the
>> recovery_target parameter set. Are we happy with the user interface
>> choices in the patch, given the understanding that the situation was
>> more comple than at first thought?
>
> Could you summarize the current proposal(s)?
>
> Personally, I don't immediately see the need to change anything from the
> parameter names that I currently see in recovery.conf.sample.

New patch version implementing everything you requested, incl docs and
tap tests.

The patch as offered here is what I've been asked to do by everybody
as well as I can do it. I'm very happy to receive comments and to
rework the design based upon further feedback.

I'm not completely convinced this is a great design, so I'm happy to
hear input. pg_basebackup -R is the main wrinkle.

The timeline handling has a bug at present that I'm working on, but
I'm not worried it constitutes a major problem. Obviously it will be
fixed before commit, but the patch needs more discussion
now/yesterday.

All parameters are set at PGC_POSTMASTER for now.

-- 
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 Fri, Feb 24, 2017 at 7:39 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
> New patch version implementing everything you requested, incl docs and
> tap tests.

The TAP changes look good to me. I thought that more diffs would have
been necessary.

> The patch as offered here is what I've been asked to do by everybody
> as well as I can do it. I'm very happy to receive comments and to
> rework the design based upon further feedback.

This meritates a summary after all has happened on this thread and
this topic. After reading this patch, here is what this is doing:
- The existing standby_mode in recovery.conf is removed, replaced by
standby.trigger to decide if a node in recovery should be put in
standby mode.
- recovery.trigger can be used to put a node in recovery. When both
standby.trigger and recovery.trigger are specified, standby.trigger
takes priority.
- Two GUC parameters, recovery_target_type and recovery_target_value,
replace all the existing recovery target parameters.
- pg_basebackup -R generates recovery.conf.auto.
- trigger_file is removed.
FWIW, my only complain is about the removal of trigger_file, this is
useful to detect a trigger file on a different partition that PGDATA!
Keeping it costs also nothing..

> I'm not completely convinced this is a great design, so I'm happy to
> hear input. pg_basebackup -R is the main wrinkle.

Yeah, I can imagine that. It is easy to append a new file to a
tarball, harder to add data to an existing file perhaps?

> The timeline handling has a bug at present that I'm working on, but
> I'm not worried it constitutes a major problem. Obviously it will be
> fixed before commit, but the patch needs more discussion
> now/yesterday.

Running the tests I can see failures in 004_timeline_switch.pl, which
is what you likely mention here, as well as a failure in
008_fsm_truncation.pl. I can also see that 009_twophase.pl is
freezing.

> All parameters are set at PGC_POSTMASTER for now.

Thanks for considering that, this makes the review of this patch
easier, and that's complicated enough as-is.

-test_recovery_standby('XID + time + name + LSN',
-   'standby_9', $node_master, \@recovery_params, "5000", $lsn5);
+# Tests for multiple targets no longer needed from 10.0 onwards
I would be fine without any comments as well. What's the point of
mentioning a past state here?
# Switch standby 2 to replay from standby 1
-rmtree($node_standby_2->data_dir . '/recovery.conf');
+#----still needed?    rmtree($node_standby_2->data_dir . '/recovery.conf');my $connstr_1 = $node_standby_1->connstr;
No need to worry here, this can be removed.

osx:func.sgml:18159:19:X: reference to non-existent ID "RECOVERY-TARGET-NAME"
osx:release-9.1.sgml:9814:23:X: reference to non-existent ID
"RECOVERY-TARGET-NAME"
osx:recovery-config.sgml:311:26:X: reference to non-existent ID
"RECOVERY-TARGET-XID"
osx:recovery-config.sgml:310:43:X: reference to non-existent ID
"RECOVERY-TARGET-TIME"
osx:release-9.4.sgml:8034:27:X: reference to non-existent ID "RECOVERY-TARGET"
Documentation fails to compile.

+    The database server can also be started "in recovery" a term that covers
+    using the server as a standby or for executing a targeted
recovery. Typically
+    standby mode would be used to provide high availability and/or read
+    scalability, whereas a targeted recovery is used to recover from data loss.
Double quotes directly used in a documentation paragraph are not nice.
You should add a comma after "in recovery".

+    To start the server in standby mode create a zero-length file
+    called <filename>standby.signal</><indexterm><primary>standby.signal</></>
Zero-length file is not mandatory. Only creating a file is.

+     <para>
+      Parameter settings may be changed in
<filename>postgresql.conf</filename> or
+      by executing the <command>ALTER SYSTEM</command>. Changes will take some
+      time to take effect, so changes made while not in paused state may not
+      have the desired effect in all cases.
+     </para>
But those parameters are PGC_POSTMASTER?!

+      By default, recovery will recover to the end of the WAL log. An earlier
+      stopping point may be specified using <varname>recovery_target_type</>
+      and in most cases also <varname>recovery_target_value</>, plus
the optional
+      parameters <varname>recovery_target_inclusive</>,
+      <varname>recovery_targeti_timeline</> and
<varname>recovery_target_action</>.
+     </para>
"recovery will recover" is a bad phrasing, I would change that to
"recovery will process".
There is also a typo => s/targeti/target/.

+     <para>
+      Parameter settings may be changed only at server start, though later
+      patches may add this capability.
+     </para>
I would just say that "new values for those parameters are considered
only at restart of the server". There is no need to speculate about a
potential future in the documentation. If nothing happens this would
remain incorrect.
        By default, a standby server restores WAL records from the
-        primary as soon as possible. It may be useful to have a time-delayed
+        sending server as soon as possible. It may be useful to have
a time-delayed
Perhaps that's a separate patch? Cascading servers can have a delay even now.

Perhaps it would be worth mentioning promote.signal in the docs?

- * recoveryTargetTLI: the desired timeline that we want to end in.
+ * recoveryTargetTimeLineGoal: what the user requested, if any
+ *
+ * recoveryTargetTLIRequested: numeric value of requested timeline, if constant
+ *
+ * recoveryTargetTLI: the currently understood target timeline; changes
Hm.. This looks like too much complication...

+       fd = BasicOpenFile(StandbySignalFile, O_RDWR | PG_BINARY |
get_sync_bit(sync_method),
+                           S_IRUSR | S_IWUSR);
+       pg_fsync(fd);
+       close(fd);
+       standby_signal_file_found = true;
Forgetting to sync the parent directory here, no?

+   int normal_log_level = LOG; //DEBUG2;
Let's not forget that.

+   if (unlink(StandbySignalFile) != 0 && standby_signal_file_found)
+       ereport(ERROR,
+           (errcode_for_file_access(),
+            errmsg("could not remove file \"%s\": %m",
+                   StandbySignalFile)));
+   if (unlink(RecoverySignalFile) != 0 && recovery_signal_file_found)
+       ereport(ERROR,
+           (errcode_for_file_access(),
+            errmsg("could not remove file \"%s\": %m",
+                   RecoverySignalFile)));
unlink() is not durable... It may be better to rename those files
using durable_rename() so as it is durable.

-   if (strlen(restore_name_str) >= MAXFNAMELEN)
+   if (strlen(restore_name_str) >= MAXRESTOREPOINTNAMELEN
Not sure there is much point to have a new variable here.

+                   /*
+                    * If primary_conninfo has been changed while
walreceiver is running,
+                    * shut down walreceiver so that a new walreceiver
is started and
+                    * initiates replication with the new connection
information.
+                    */
+                   if (strcmp(conninfo, PrimaryConnInfo) != 0)
+                       ereport(FATAL,
+                               (errcode(ERRCODE_ADMIN_SHUTDOWN),
+                                errmsg("closing replication
connection because primary_conninfo was changed")));
This cannot happen, those are postmaster parameters. Let's remove any
complications if those are not needed.

+       if (!ParseConfigFile(RECOVERY_AUTOCONF_FILENAME, false,
+                            NULL, 0, 0, elevel,
+                            &head, &tail))
+       {
+           /* Syntax error(s) detected in the file, so bail out */
+           error = true;
+           ConfFileWithError = RECOVERY_AUTOCONF_FILENAME;
+           goto bail_out;
+       }
Surely this should be documented at least on pg_backbackup page.

+   else if (strcmp(*newval, "controlfile") == 0 || strcmp(*newval, "") == 0)
+       rttg = RECOVERY_TARGET_TIMELINE_CONTROLFILE;
What the idea behind "controlfile" as value for recovery_target_timeline?

+# - Archive Recovery -
+#restore_command = ''      # command to use to restore an archived
logfile segment
+               # placeholders: %p = path of file to restore
+               #               %f = file name only
+               # e.g. 'cp /mnt/server/archivedir/%f %p
It may be good to mention that those parameters are ignored on a primary server.


+                        *
+                        * Note we don't skip STANDBY_SIGNAL_FILE (correct??)                        */
My guess is that those should be kept in backups by default, and
dropped if -R is used.

+#define RecoveryTargetText(t) ( \
+   t == RECOVERY_TARGET_UNSET ? "unset" : ( \
+   t == RECOVERY_TARGET_XID   ? "xid" : ( \
+   t == RECOVERY_TARGET_TIME  ? "timestamp" : ( \
+   t == RECOVERY_TARGET_NAME  ? "name" : ( \
+   t == RECOVERY_TARGET_LSN   ? "lsn" : \
+                   "immediate" )))))
The default value should be "none", not "immediate".

Perhaps it is time to introduce a xlogrecovery.c and reduce again the
size of xlog.c...
--
Michael

On 25 February 2017 at 13:58, Michael Paquier <michael.paquier@gmail.com> wrote:

> - trigger_file is removed.
> FWIW, my only complain is about the removal of trigger_file, this is
> useful to detect a trigger file on a different partition that PGDATA!
> Keeping it costs also nothing..

Sorry, that is just an error of implementation, not intention. I had
it on my list to keep, at your request.

New version coming up.

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

On Wed, Jan 11, 2017 at 11:23 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
>> I think the issue was that some people didn't want configuration files
>> in the data directory.  By removing recovery.conf we accomplish that.
>> Signal/trigger files are not configuration (or at least it's much easier
>> to argue that), so I think having them in the data directory is fine.
>
> There were a considerable number of people that pushed to make the
> data directory non-user writable, which is where the signal directory
> came from.

Specifically, it's a problem for Debian's packaging conventions,
right?  The data directory can contain anything that the server itself
will write, but configuration files that are written for the server to
read are supposed to go in some external location dictated by Debian's
packaging policy.

Things like trigger files aren't configuration files per se, so maybe
it's OK if those still get written into the data directory.  Even if
not, that seems like a separate patch.  In my view, based on Michael's
description of what the current patch version does, it's a clear step
forward.  Other steps can be taken at another time, if required.

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

On Sat, Feb 25, 2017 at 7:28 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> - pg_basebackup -R generates recovery.conf.auto.

Does anything cause that file to get read?

Wouldn't it be better to just append to postgresql.conf.auto?

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

On 02/26/2017 12:55 AM, Robert Haas wrote:
> On Wed, Jan 11, 2017 at 11:23 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
>>> I think the issue was that some people didn't want configuration files
>>> in the data directory.  By removing recovery.conf we accomplish that.
>>> Signal/trigger files are not configuration (or at least it's much easier
>>> to argue that), so I think having them in the data directory is fine.
>>
>> There were a considerable number of people that pushed to make the
>> data directory non-user writable, which is where the signal directory
>> came from.
> 
> Specifically, it's a problem for Debian's packaging conventions,
> right?  The data directory can contain anything that the server itself
> will write, but configuration files that are written for the server to
> read are supposed to go in some external location dictated by Debian's
> packaging policy.
> 
> Things like trigger files aren't configuration files per se, so maybe
> it's OK if those still get written into the data directory.  Even if
> not, that seems like a separate patch.  In my view, based on Michael's
> description of what the current patch version does, it's a clear step
> forward.  Other steps can be taken at another time, if required.
> 

From the perspective of containerized Postgres, you want config files to
go into one (non-writeable) directory, and anything which is writeable
by the DB server to go into another directory (and preferably, a single
directory).

A trigger file (that is, assuming an empty one, and recovery config
merged with pg.conf) would thus be writeable, non-configuration data
which goes in the data directory.

Users manually writing the trigger file doesn't show up as a problem
since, in a containerized environment, they can't.  It's either written
by postgres itself, or by management software which runs as the postgres
user.

-- 
Josh Berkus
Containers & Databases Oh My!

On Sun, Feb 26, 2017 at 5:56 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Sat, Feb 25, 2017 at 7:28 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> - pg_basebackup -R generates recovery.conf.auto.
>
> Does anything cause that file to get read?
>
> Wouldn't it be better to just append to postgresql.conf.auto?

Yeah, that would be cleaner than having the backend look for an extra
hardcoded path. Looking at pg_basebackup.c, actually it would not be
difficult to append data to an existing file: look for the file in the
tar stream, and when it is here save its content for later and bypass
it. Once the tar stream is written, just use the data saved previously
and append the parameters at the end of it.
-- 
Michael

Hi Simon,

On 2/25/17 2:43 PM, Simon Riggs wrote:
> On 25 February 2017 at 13:58, Michael Paquier <michael.paquier@gmail.com> wrote:
> 
>> - trigger_file is removed.
>> FWIW, my only complain is about the removal of trigger_file, this is
>> useful to detect a trigger file on a different partition that PGDATA!
>> Keeping it costs also nothing..
> 
> Sorry, that is just an error of implementation, not intention. I had
> it on my list to keep, at your request.
> 
> New version coming up.

Do you have an idea when the new version will be available?

Thanks,
-- 
-David
david@pgmasters.net

On 03/02/2017 07:13 AM, David Steele wrote:
> Hi Simon,
> 
> On 2/25/17 2:43 PM, Simon Riggs wrote:
>> On 25 February 2017 at 13:58, Michael Paquier <michael.paquier@gmail.com> wrote:
>>
>>> - trigger_file is removed.
>>> FWIW, my only complain is about the removal of trigger_file, this is
>>> useful to detect a trigger file on a different partition that PGDATA!
>>> Keeping it costs also nothing..
>>
>> Sorry, that is just an error of implementation, not intention. I had
>> it on my list to keep, at your request.
>>
>> New version coming up.
> 
> Do you have an idea when the new version will be available?

Please?  Having yet another PostgreSQL release go by without fixing
recovery.conf would make me very sad.


-- 
Josh Berkus
Containers & Databases Oh My!

On 7 March 2017 at 23:31, Josh Berkus <josh@berkus.org> wrote:
> On 03/02/2017 07:13 AM, David Steele wrote:
>> Hi Simon,
>>
>> On 2/25/17 2:43 PM, Simon Riggs wrote:
>>> On 25 February 2017 at 13:58, Michael Paquier <michael.paquier@gmail.com> wrote:
>>>
>>>> - trigger_file is removed.
>>>> FWIW, my only complain is about the removal of trigger_file, this is
>>>> useful to detect a trigger file on a different partition that PGDATA!
>>>> Keeping it costs also nothing..
>>>
>>> Sorry, that is just an error of implementation, not intention. I had
>>> it on my list to keep, at your request.
>>>
>>> New version coming up.
>>
>> Do you have an idea when the new version will be available?
>
> Please?  Having yet another PostgreSQL release go by without fixing
> recovery.conf would make me very sad.

I share your pain, but there are various things about this patch that
make me uncomfortable. I believe we are looking for an improved design
not just a different design.

I think the best time to commit such a patch is at the beginning of a
new cycle, so people have a chance to pick out pieces they don't like
and incrementally propose changes.

So I am going to mark this MovedToNextCF, barring objections from
committers willing to make it happen in this release.

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

On Mon, Mar 27, 2017 at 5:27 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
> I share your pain, but there are various things about this patch that
> make me uncomfortable. I believe we are looking for an improved design
> not just a different design.
>
> I think the best time to commit such a patch is at the beginning of a
> new cycle, so people have a chance to pick out pieces they don't like
> and incrementally propose changes.
>
> So I am going to mark this MovedToNextCF, barring objections from
> committers willing to make it happen in this release.

+1.
-- 
Michael

> On 27 Mar 2017, at 10:27, Simon Riggs <simon@2ndquadrant.com> wrote:
>
> On 7 March 2017 at 23:31, Josh Berkus <josh@berkus.org> wrote:
>> On 03/02/2017 07:13 AM, David Steele wrote:
>>> Hi Simon,
>>>
>>> On 2/25/17 2:43 PM, Simon Riggs wrote:
>>>> On 25 February 2017 at 13:58, Michael Paquier <michael.paquier@gmail.com> wrote:
>>>>
>>>>> - trigger_file is removed.
>>>>> FWIW, my only complain is about the removal of trigger_file, this is
>>>>> useful to detect a trigger file on a different partition that PGDATA!
>>>>> Keeping it costs also nothing..
>>>>
>>>> Sorry, that is just an error of implementation, not intention. I had
>>>> it on my list to keep, at your request.
>>>>
>>>> New version coming up.
>>>
>>> Do you have an idea when the new version will be available?
>>
>> Please?  Having yet another PostgreSQL release go by without fixing
>> recovery.conf would make me very sad.
>
> I share your pain, but there are various things about this patch that
> make me uncomfortable. I believe we are looking for an improved design
> not just a different design.
>
> I think the best time to commit such a patch is at the beginning of a
> new cycle, so people have a chance to pick out pieces they don't like
> and incrementally propose changes.
>
> So I am going to mark this MovedToNextCF, barring objections from
> committers willing to make it happen in this release.

Next CF has now become This CF, has there been any work on this highly sought
after patch?  Would be good to get closure on this early in the v11 cycle.

cheers ./daniel

On 5 September 2017 at 06:47, Daniel Gustafsson <daniel@yesql.se> wrote:
>> On 27 Mar 2017, at 10:27, Simon Riggs <simon@2ndquadrant.com> wrote:
>>
>> On 7 March 2017 at 23:31, Josh Berkus <josh@berkus.org> wrote:
>>> On 03/02/2017 07:13 AM, David Steele wrote:
>>>> Hi Simon,
>>>>
>>>> On 2/25/17 2:43 PM, Simon Riggs wrote:
>>>>> On 25 February 2017 at 13:58, Michael Paquier <michael.paquier@gmail.com> wrote:
>>>>>
>>>>>> - trigger_file is removed.
>>>>>> FWIW, my only complain is about the removal of trigger_file, this is
>>>>>> useful to detect a trigger file on a different partition that PGDATA!
>>>>>> Keeping it costs also nothing..
>>>>>
>>>>> Sorry, that is just an error of implementation, not intention. I had
>>>>> it on my list to keep, at your request.
>>>>>
>>>>> New version coming up.
>>>>
>>>> Do you have an idea when the new version will be available?
>>>
>>> Please?  Having yet another PostgreSQL release go by without fixing
>>> recovery.conf would make me very sad.
>>
>> I share your pain, but there are various things about this patch that
>> make me uncomfortable. I believe we are looking for an improved design
>> not just a different design.
>>
>> I think the best time to commit such a patch is at the beginning of a
>> new cycle, so people have a chance to pick out pieces they don't like
>> and incrementally propose changes.
>>
>> So I am going to mark this MovedToNextCF, barring objections from
>> committers willing to make it happen in this release.
>
> Next CF has now become This CF, has there been any work on this highly sought
> after patch?  Would be good to get closure on this early in the v11 cycle.

I've not worked on this at all, so it isn't ready for re-review and commit.

I get the "lets do it early" thing and will try to extract a subset in October.

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

On Wed, Sep 6, 2017 at 12:31 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
> I've not worked on this at all, so it isn't ready for re-review and commit.
>
> I get the "lets do it early" thing and will try to extract a subset in October.

Nice to see that you are still planning to work on that. I would
suggest to move this item to the next open CF then, with "waiting on
author" as status.
-- 
Michael