Обсуждение: could recovery_target_timeline=latest be the default in standby mode?

Is there ever a reason why you would *not* want
recovery_target_timeline=latest in standby mode?

pg_basebackup -R doesn't set it.  That seems suboptimal.

Perhaps this could be the default in standby mode, or even the implicit
default, ignoring the recovery_target_timeline setting altogether.

How could we make things simpler here?

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

Hello

I am +1 for recovery_target_timeline=latest by default. This is common case in my opinion. And first release without
recovery.confis reasonable time for change default value.
 

But i doubt if we can ignore recovery_target_timeline in standby and always use latest in standby. I have no use case
forthis but i prefer change only default value.
 

regards, Sergei

On Fri, Dec 21, 2018 at 01:54:20PM +0300, Sergei Kornilov wrote:
> I am +1 for recovery_target_timeline=latest by default. This is
> common case in my opinion.

I agree also that switching to the latest timeline should be the
default.  People get confused because of the current default.
--
Michael

On 22/12/2018 00:38, Michael Paquier wrote:
> On Fri, Dec 21, 2018 at 01:54:20PM +0300, Sergei Kornilov wrote:
>> I am +1 for recovery_target_timeline=latest by default. This is
>> common case in my opinion.
> 
> I agree also that switching to the latest timeline should be the
> default.  People get confused because of the current default.

How about this patch then?

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

On 12/27/18 1:02 PM, Peter Eisentraut wrote:
> On 22/12/2018 00:38, Michael Paquier wrote:
>> On Fri, Dec 21, 2018 at 01:54:20PM +0300, Sergei Kornilov wrote:
>>> I am +1 for recovery_target_timeline=latest by default. This is
>>> common case in my opinion.
>>
>> I agree also that switching to the latest timeline should be the
>> default.  People get confused because of the current default.
> 
> How about this patch then?

I like the idea of defaulting to the latest timeline since this is what 
you want to do most of the time, but I think we'd then need a value for 
following the current timelime, i.e. the one that the backup was taken 
on (which is the current default).

I also think that changing the behavior of this setting based on 
standby_mode is going to be confusing.  Recovering to the most recent 
timeline is the general case whether setting up a standby or not.

Imagine the following case:

1) Primary fails
2) Switch to standby
3) Standby runs for a while but fails before the old primary is rebuilt
4) We recover a new primary from the most recent backup which is 
probably on the old timeline, but we'd rather recover to the new 
timeline established after the failover.

Or, we recover a cluster for reporting and promote it so we can create 
temp tables.  We'd also like that to be on the most recent timeline.

I would recommend:

1) Make the default for recovery_target_timeline always be latest.
2) Add a new value, current, that replicates the current behavior.

Regards,
-- 
-David
david@pgmasters.net

On Thu, Dec 27, 2018 at 06:36:23PM +0200, David Steele wrote:
> I like the idea of defaulting to the latest timeline since this is what you
> want to do most of the time, but I think we'd then need a value for
> following the current timelime, i.e. the one that the backup was taken on
> (which is the current default).
>
> [...]
>
> I would recommend:
>
> 1) Make the default for recovery_target_timeline always be latest.
> 2) Add a new value, current, that replicates the current behavior.

Yes, I was also thinking something among those lines, and the patch is
a bit confusing by linking standby mode with latest timeline.  It
seems to me that switching the default value to "latest" at GUC level
would be the way to go, instead of picking up the TLI from the control
file.  Introducing a new value which maps to the current empty value
may be useful as well, like "control_file"?
--
Michael

On 28/12/2018 00:15, Michael Paquier wrote:
> Yes, I was also thinking something among those lines, and the patch is
> a bit confusing by linking standby mode with latest timeline.  It
> seems to me that switching the default value to "latest" at GUC level
> would be the way to go, instead of picking up the TLI from the control
> file.  Introducing a new value which maps to the current empty value
> may be useful as well, like "control_file"?

OK, here are patches for this approach:

1. Add value 'current' for recovery_target_timeline
2. Change default of recovery_target_timeline to 'latest'

The first is really a fixup of the recovery.conf-removal patch.  In
<=PG11, you could not explicitly select the current timeline; it was
only available if you don't mention recovery_target_timeline at all.
The original patch contained a setting 'controlfile', similar to your
suggestion, but that sounds a bit low-level implementation detail to me.
 I like the suggestion 'current'.

The second then just changes the GUC default, without any special
treatment for standby mode.

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

Hi Peter,

On 12/28/18 1:08 PM, Peter Eisentraut wrote:
> On 28/12/2018 00:15, Michael Paquier wrote:
>> Yes, I was also thinking something among those lines, and the patch is
>> a bit confusing by linking standby mode with latest timeline.  It
>> seems to me that switching the default value to "latest" at GUC level
>> would be the way to go, instead of picking up the TLI from the control
>> file.  Introducing a new value which maps to the current empty value
>> may be useful as well, like "control_file"?
> 
> OK, here are patches for this approach:
> 
> 1. Add value 'current' for recovery_target_timeline
> 2. Change default of recovery_target_timeline to 'latest'
> 
> The first is really a fixup of the recovery.conf-removal patch.  In
> <=PG11, you could not explicitly select the current timeline; it was
> only available if you don't mention recovery_target_timeline at all.
> The original patch contained a setting 'controlfile', similar to your
> suggestion, but that sounds a bit low-level implementation detail to me.
>   I like the suggestion 'current'.

This patch looks good to me.

> > The second then just changes the GUC default, without any special
> treatment for standby mode.

Yes, that's exactly what I was thinking.

There don't seem to be any tests for recovery_target_timeline=current. 
This is an preexisting condition but it probably wouldn't hurt to add one.

Regards,
-- 
-David
david@pgmasters.net

On Mon, Dec 31, 2018 at 01:21:00PM +0200, David Steele wrote:
> This patch looks good to me.

(Sorry for the delay here)
0001 looks fine to me.

-        to the latest timeline found in the archive, which is useful in
-        a standby server. Other than that you only need to set this parameter
+        to the latest timeline found in the archive.  That is the default.
+       </para>
Isn't it useful to still mention that the default is useful especially
for standby servers?

-    the WAL archive. If you plan to have multiple standby servers for high
-    availability purposes, set <varname>recovery_target_timeline</varname> to
-    <literal>latest</literal>, to make the standby server follow the timeline change
-    that occurs at failover to another standby.
+    the WAL archive.
I think that we should still keep this recommendation as well, as well
as the one below.

-   RecoveryTargetTimeLineGoal rttg = RECOVERY_TARGET_TIMELINE_CONTROLFILE;
+   RecoveryTargetTimeLineGoal rttg;
Good to remove this noise.

> Yes, that's exactly what I was thinking.

Agreed.

> There don't seem to be any tests for recovery_target_timeline=current. This
> is an preexisting condition but it probably wouldn't hurt to add one.

Yes, I got to wonder while looking at this patch why we don't have one
yet in 003_recovery_targets.pl.  That's easy enough to do thanks to
the extra rows inserted after doing the stuff for the LSN-based
restart point, and attached is a patch to add the test.  Peter, could
you merge it with 0001?  I am fine to take care of that myself if
necessary.
--
Michael

On 1/8/19 5:37 AM, Michael Paquier wrote:
> 
> -        to the latest timeline found in the archive, which is useful in
> -        a standby server. Other than that you only need to set this parameter
> +        to the latest timeline found in the archive.  That is the default.
> +       </para>
> Isn't it useful to still mention that the default is useful especially
> for standby servers?

Agreed.

> -    the WAL archive. If you plan to have multiple standby servers for high
> -    availability purposes, set <varname>recovery_target_timeline</varname> to
> -    <literal>latest</literal>, to make the standby server follow the timeline change
> -    that occurs at failover to another standby.
> +    the WAL archive.
> I think that we should still keep this recommendation as well, as well
> as the one below.

Agreed.

> 
>> There don't seem to be any tests for recovery_target_timeline=current. This
>> is an preexisting condition but it probably wouldn't hurt to add one.
> 
> Yes, I got to wonder while looking at this patch why we don't have one
> yet in 003_recovery_targets.pl.  That's easy enough to do thanks to
> the extra rows inserted after doing the stuff for the LSN-based
> restart point, and attached is a patch to add the test.  Peter, could
> you merge it with 0001?  I am fine to take care of that myself if
> necessary.

The new test looks good.

Regards,
-- 
-David
david@pgmasters.net

On 08/01/2019 04:37, Michael Paquier wrote:
> On Mon, Dec 31, 2018 at 01:21:00PM +0200, David Steele wrote:
>> This patch looks good to me.
> 
> 0001 looks fine to me.

committed that one

> -        to the latest timeline found in the archive, which is useful in
> -        a standby server. Other than that you only need to set this parameter
> +        to the latest timeline found in the archive.  That is the default.
> +       </para>
> Isn't it useful to still mention that the default is useful especially
> for standby servers?
> 
> -    the WAL archive. If you plan to have multiple standby servers for high
> -    availability purposes, set <varname>recovery_target_timeline</varname> to
> -    <literal>latest</literal>, to make the standby server follow the timeline change
> -    that occurs at failover to another standby.
> +    the WAL archive.
> I think that we should still keep this recommendation as well, as well
> as the one below.

Attached revised 0002 with those changes.

>> There don't seem to be any tests for recovery_target_timeline=current. This
>> is an preexisting condition but it probably wouldn't hurt to add one.
> 
> Yes, I got to wonder while looking at this patch why we don't have one
> yet in 003_recovery_targets.pl.  That's easy enough to do thanks to
> the extra rows inserted after doing the stuff for the LSN-based
> restart point, and attached is a patch to add the test.  Peter, could
> you merge it with 0001?  I am fine to take care of that myself if
> necessary.

In that test, if I change the 'current' to 'latest', the test doesn't
fail, so it's probably not a good test.

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

On Fri, Jan 11, 2019 at 11:17:48AM +0100, Peter Eisentraut wrote:
> Attached revised 0002 with those changes.

This one looks fine.

> In that test, if I change the 'current' to 'latest', the test doesn't
> fail, so it's probably not a good test.

I can see your point.  You would need a diverging timeline to test for
'latest', which can surely be done as part of 003_recovery_targets.pl.
It seems to me that that the test has initial value to make sure that
we replay up to the end of the produced timeline's data, which is
something untested now as the script has only restart points set to
before the end of the timeline.  If you think that's not a good
addition now, I am also fine to not include it.
--
Michael

On 12/01/2019 00:53, Michael Paquier wrote:
> On Fri, Jan 11, 2019 at 11:17:48AM +0100, Peter Eisentraut wrote:
>> Attached revised 0002 with those changes.
> 
> This one looks fine.

committed

>> In that test, if I change the 'current' to 'latest', the test doesn't
>> fail, so it's probably not a good test.
> 
> I can see your point.  You would need a diverging timeline to test for
> 'latest', which can surely be done as part of 003_recovery_targets.pl.
> It seems to me that that the test has initial value to make sure that
> we replay up to the end of the produced timeline's data, which is
> something untested now as the script has only restart points set to
> before the end of the timeline.  If you think that's not a good
> addition now, I am also fine to not include it.

I'm not sure what the coverage is in detail in this area.  It seems we
already have tests for not-specific-recovery-target, maybe not in this
file, but most of the other tests rely on that, no?

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

On Sun, Jan 13, 2019 at 10:17:32AM +0100, Peter Eisentraut wrote:
> I'm not sure what the coverage is in detail in this area.  It seems we
> already have tests for not-specific-recovery-target, maybe not in this
> file, but most of the other tests rely on that, no?

[... Checking ...]
There are no tests on HEAD using directly recovery_target_timeline in
any .pl or .pm file, which is definitely not a good idea.
004_timeline_switch.pl is actually designed for checking the latest
timeline, which is fine.  002_archiving.pl somewhat tests for the
current timeline, still it is not really designed for this purpose.
--
Michael