Re: [proposal] recovery_target "latest"

[Tomas Vondra]

Hi,

this patch was waiting on author without any update/response since early
December, so I've marked it as returned with feedback. Feel free to
re-submit an updated version to a future CF.

regards

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

Re: [proposal] recovery_target "latest"

[Kyotaro Horiguchi]

Thanks for the new version.

At Sun, 8 Dec 2019 04:03:01 +0300, Grigory Smolkin  
wrote in 
> On 11/21/19 1:46 PM, Peter Eisentraut wrote:
> > On 2019-11-08 05:00, Grigory Smolkin wrote:
> I`ve tested it and have some thoughts/concerns:
> 
> 1. Recovery should report the exact reason why it has been forced to
> stop. In case of recovering to the end of WAL, standby promotion
> request received during recovery could be mistaken for reaching the
> end of WAL and reported as such. To avoid this, I think that
> reachedEndOfWal variable should be introduced.
>
> In attached patch it is added as a global variable, but maybe
> something more clever may be devised. I was not sure that
> reachedEndOfWal could be placed in XLogPageReadPrivate. Because we
> need to access it at the higher level than ReadRecord(), and I was
> under impression placing it in XLogPageReadPrivate could violate
> abstraction level of XLogPageReadPrivate.

CheckForStandbyTrigger() always returns true once the trigger is
pulled. We don't care whether end-of-WAL is reached if promote is
already triggered. Thus, we can tell the promote case by asking
CheckForStandbyTrigger() when we exited the redo main loop with
recoveryTarget = RECOVERY_TARGET_LATEST. Is this works as you expect?

> 2. During the testing, I`ve stumbled upon assertion failure in case of
> recovering in standby mode to the the end of WAL coupled with
> recovery_target_action as "promote", caused by the WAL source in state
> machine not been changed after reaching the recovery target (script to
> reproduce is attached):
...
> TRAP: FailedAssertion("StandbyMode", File: "xlog.c", Line: 12032)
...
> #2  0x00a88b82 in ExceptionalCondition (conditionName=0xb24acc
> #"StandbyMode", errorType=0xb208a7 "FailedAssertion",
>     fileName=0xb208a0 "xlog.c", lineNumber=12032) at assert.c:67
> #3  0x00573417 in WaitForWALToBecomeAvailable
> #(RecPtr=151003136, randAccess=true, fetching_ckpt=false,
> #tliRecPtr=167757424,
>     return_on_eow=true) at xlog.c:12032
...
> #7  0x005651f8 in ReadRecord (xlogreader=0xf08ed8,
> #RecPtr=167757424, emode=22, fetching_ckpt=false) at xlog.c:4271
..

ReadRecord is called with currentSource=STREAM after StandbyMode was
turned off. I suppose the fix means the "currentSource =
XLOG_FROM_PG_WAL" line but I don't think it not the right way.

Streaming timeout means failure when return_on_eow is true. Thus the
right thing to do there is setting lastSourceFailed to true. The first
half of WaitForWALToBecomeAvailable handles failure of the current
source thus source transition happens only there. The second half just
reports failure to the first half.

> Both issues are fixed in the new patch version.
> Any review and thoughts on the matters would be much appreciated.
> 
> 
> >
> > I think The doc needs to exiplain on the difference between default
> > and latest.
> Sure, I will work on it.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: [proposal] recovery_target "latest"

[Grigory Smolkin]

On 11/21/19 1:46 PM, Peter Eisentraut wrote:

On 2019-11-08 05:00, Grigory Smolkin wrote:

Attached new patch revision, now end of available WAL is defined as the
fact of missing required WAL.
In case of standby, the end of WAL is defined as 2 consecutive switches
of WAL source, that didn`t provided requested record.
In case of streaming standby, each switch of WAL source is forced after
3 failed attempts to get new data from walreceiver.

All constants are taken off the top of my head and serves as proof of
concept.


Well, this is now changing the meaning of the patch quite a bit. I'm 
on board with making the existing default behavior explicit. (This is 
similar to how we added recovery_target_timeline = 'current' in PG12.) 
Still not a fan of the name yet, but that's trivial.


If, however, you want to change the default behavior or introduce a 
new behavior, as you are suggesting here, that should be a separate 
discussion.


No, default behavior is not to be changed. As I previously mentioned, it 
may break the backward compatibility for 3rd party backup and 
replication applications.



At Fri, 8 Nov 2019 16:08:47 +0300, Grigory Smolkin  
wrote in

On 11/8/19 7:00 AM, Grigory Smolkin wrote:

On 11/7/19 4:36 PM, Grigory Smolkin wrote:

I gave it some thought and now think that prohibiting recovery_target
'latest' and standby was a bad idea.
All recovery_targets follow the same pattern of usage, so
recovery_target "latest" also must be capable of working in standby
mode.
All recovery_targets have a clear deterministic 'target' where
recovery should end.
In case of recovery_target "latest" this target is the end of
available WAL, therefore the end of available WAL must be more clearly
defined.
I will work on it.

Thank you for a feedback.

Attached new patch revision, now end of available WAL is defined as
the fact of missing required WAL.
In case of standby, the end of WAL is defined as 2 consecutive
switches of WAL source, that didn`t provided requested record.
In case of streaming standby, each switch of WAL source is forced
after 3 failed attempts to get new data from walreceiver.

All constants are taken off the top of my head and serves as proof of
concept.
TAP test is updated.


Attached new revision, it contains some minor refactoring.

Thanks for the new patch. I found that it needs more than I thought,
but it seems a bit too complicated and less stable.

As the patch does, WaitForWALToBecomeAvailable needs to exit when
avaiable sources are exhausted. However, we don't need to count
failures to do that. It is enough that the function have two more exit
point. When streaming timeout fires, and when we found that streaming
is not set up when archive/wal failed.

In my opinion, it is better that we have less dependency to global
variables in such deep levels in a call hierachy. Such nformation can
be stored in XLogPageReadPrivate.


Many thanks!
It looks much better that my approach with global variables.

I`ve tested it and have some thoughts/concerns:

1. Recovery should report the exact reason why it has been forced to 
stop. In case of recovering to the end of WAL, standby promotion request 
received during recovery could be mistaken for reaching the end of WAL 
and reported as such. To avoid this, I think that reachedEndOfWal 
variable should be introduced.


In attached patch it is added as a global variable, but maybe something 
more clever may be devised. I was not sure that reachedEndOfWal could be 
placed in XLogPageReadPrivate. Because we need to access it at the 
higher level than ReadRecord(), and I was under impression placing it in 
XLogPageReadPrivate could violate abstraction level of XLogPageReadPrivate.


2. During the testing, I`ve stumbled upon assertion failure in case of 
recovering in standby mode to the the end of WAL coupled with 
recovery_target_action as "promote", caused by the WAL source in state 
machine not been changed after reaching the recovery target (script to 
reproduce is attached):


2019-12-07 13:45:54.468 MSK [23067] LOG:  starting PostgreSQL 13devel on 
x86_64-pc-linux-gnu, compiled by gcc (GCC) 9.2.1 20190827 (Red Hat 
9.2.1-1), 64-bit
2019-12-07 13:45:54.468 MSK [23067] LOG:  listening on IPv4 address 
"127.0.0.1", port 15433
2019-12-07 13:45:54.470 MSK [23067] LOG:  listening on Unix socket 
"/tmp/.s.PGSQL.15433"
2019-12-07 13:45:54.475 MSK [23068] LOG:  database system was 
interrupted; last known up at 2019-12-07 13:45:49 MSK
cp: cannot stat '/home/gsmol/task/13_devel/archive/0002.history': No 
such file or directory

2019-12-07 13:45:54.602 MSK [23068] LOG:  entering standby mode
2019-12-07 13:45:54.614 MSK [23068] LOG:  restored log file 
"00010002" from archive

2019-12-07 13:45:54.679 MSK [23068] LOG:  redo starts at 0/228
2019-12-07 13:45:54.682 MSK [23068] LOG:  consistent recovery state 
reached at 0/2000100
2019-12-07 13:45:54.682 MSK [23067] LOG:  database system is ready to 
accept read only connections
2019-12-07 

Re: [proposal] recovery_target "latest"

[Peter Eisentraut]

On 2019-11-08 05:00, Grigory Smolkin wrote:

Attached new patch revision, now end of available WAL is defined as the
fact of missing required WAL.
In case of standby, the end of WAL is defined as 2 consecutive switches
of WAL source, that didn`t provided requested record.
In case of streaming standby, each switch of WAL source is forced after
3 failed attempts to get new data from walreceiver.

All constants are taken off the top of my head and serves as proof of
concept.


Well, this is now changing the meaning of the patch quite a bit.  I'm on 
board with making the existing default behavior explicit.  (This is 
similar to how we added recovery_target_timeline = 'current' in PG12.) 
Still not a fan of the name yet, but that's trivial.


If, however, you want to change the default behavior or introduce a new 
behavior, as you are suggesting here, that should be a separate discussion.


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

Re: [proposal] recovery_target "latest"

[Kyotaro Horiguchi]

At Fri, 8 Nov 2019 16:08:47 +0300, Grigory Smolkin  
wrote in 
> 
> On 11/8/19 7:00 AM, Grigory Smolkin wrote:
> >
> > On 11/7/19 4:36 PM, Grigory Smolkin wrote:
> >> I gave it some thought and now think that prohibiting recovery_target
> >> 'latest' and standby was a bad idea.
> >> All recovery_targets follow the same pattern of usage, so
> >> recovery_target "latest" also must be capable of working in standby
> >> mode.
> >> All recovery_targets have a clear deterministic 'target' where
> >> recovery should end.
> >> In case of recovery_target "latest" this target is the end of
> >> available WAL, therefore the end of available WAL must be more clearly
> >> defined.
> >> I will work on it.
> >>
> >> Thank you for a feedback.
> >
> >
> > Attached new patch revision, now end of available WAL is defined as
> > the fact of missing required WAL.
> > In case of standby, the end of WAL is defined as 2 consecutive
> > switches of WAL source, that didn`t provided requested record.
> > In case of streaming standby, each switch of WAL source is forced
> > after 3 failed attempts to get new data from walreceiver.
> >
> > All constants are taken off the top of my head and serves as proof of
> > concept.
> > TAP test is updated.
> >
> Attached new revision, it contains some minor refactoring.

Thanks for the new patch. I found that it needs more than I thought,
but it seems a bit too complicated and less stable.

As the patch does, WaitForWALToBecomeAvailable needs to exit when
avaiable sources are exhausted. However, we don't need to count
failures to do that. It is enough that the function have two more exit
point. When streaming timeout fires, and when we found that streaming
is not set up when archive/wal failed.

In my opinion, it is better that we have less dependency to global
variables in such deep levels in a call hierachy. Such nformation can
be stored in XLogPageReadPrivate.

I think The doc needs to exiplain on the difference between default
and latest.

Please find the attached, which illustrates the first two points of
the aboves.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 3b766e66b9..8c8a1c6bf0 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -808,6 +808,7 @@ typedef struct XLogPageReadPrivate
 	int			emode;
 	bool		fetching_ckpt;	/* are we fetching a checkpoint record? */
 	bool		randAccess;
+	bool		return_on_eow;  /* returns when reaching End of WAL */
 } XLogPageReadPrivate;
 
 /*
@@ -887,7 +888,9 @@ static int	XLogFileReadAnyTLI(XLogSegNo segno, int emode, int source);
 static int	XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
 		 int reqLen, XLogRecPtr targetRecPtr, char *readBuf);
 static bool WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
-		bool fetching_ckpt, XLogRecPtr tliRecPtr);
+		bool fetching_ckpt,
+		XLogRecPtr tliRecPtr,
+		bool return_on_eow);
 static int	emode_for_corrupt_record(int emode, XLogRecPtr RecPtr);
 static void XLogFileClose(void);
 static void PreallocXlogFiles(XLogRecPtr endptr);
@@ -4253,6 +4256,7 @@ ReadRecord(XLogReaderState *xlogreader, XLogRecPtr RecPtr, int emode,
 	private->fetching_ckpt = fetching_ckpt;
 	private->emode = emode;
 	private->randAccess = (RecPtr != InvalidXLogRecPtr);
+	private->return_on_eow = (recoveryTarget == RECOVERY_TARGET_LATEST);
 
 	/* This is the first attempt to read this page. */
 	lastSourceFailed = false;
@@ -4371,8 +4375,12 @@ ReadRecord(XLogReaderState *xlogreader, XLogRecPtr RecPtr, int emode,
 continue;
 			}
 
-			/* In standby mode, loop back to retry. Otherwise, give up. */
-			if (StandbyMode && !CheckForStandbyTrigger())
+			/*
+			 * In standby mode, loop back to retry. Otherwise, give up.
+			 * If we told to return on end of WAL, also give up.
+			 */
+			if (StandbyMode && !CheckForStandbyTrigger() &&
+!private->return_on_eow)
 continue;
 			else
 return NULL;
@@ -7271,7 +7279,7 @@ StartupXLOG(void)
 			 * end of main redo apply loop
 			 */
 
-			if (reachedStopPoint)
+			if (reachedStopPoint || recoveryTarget == RECOVERY_TARGET_LATEST)
 			{
 if (!reachedConsistency)
 	ereport(FATAL,
@@ -7473,6 +7481,8 @@ StartupXLOG(void)
 	 recoveryStopName);
 		else if (recoveryTarget == RECOVERY_TARGET_IMMEDIATE)
 			snprintf(reason, sizeof(reason), "reached consistency");
+		else if (recoveryTarget == RECOVERY_TARGET_LATEST)
+			snprintf(reason, sizeof(reason), "end of WAL");
 		else
 			snprintf(reason, sizeof(reason), "no recovery target specified");
 
@@ -11605,7 +11615,8 @@ retry:
 		if (!WaitForWALToBecomeAvailable(targetPagePtr + reqLen,
 		 private->randAccess,
 		 private->fetching_ckpt,
-		 targetRecPtr))
+		 targetRecPtr,
+		 private->return_on_eow))
 		{
 			if (readFile >= 0)
 close(readFile);
@@ -11745,7 +11756,9 @@ 

Re: [proposal] recovery_target "latest"

[Kyotaro Horiguchi]

At Fri, 8 Nov 2019 16:08:47 +0300, Grigory Smolkin  
wrote in 
> While working on it, I stumbled upon something strange:
> 
> why DisownLatch(>recoveryWakeupLatch) is called before
> ReadRecord(xlogreader, LastRec, PANIC, false) ?
> Isn`t this latch may be accessed in WaitForWALToBecomeAvailable() if
> streaming standby gets promoted?

The DisownLatch is just for the sake of tidiness and can be placed
anywhere after the ShutdownWalRcv() call but the current place (just
before "StandbyMode = false") seems natural. The ReadRecord call
doesn't launch another wal receiver because we cleard StandbyMode just
before.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: [proposal] recovery_target "latest"

[Grigory Smolkin]

On 11/8/19 7:00 AM, Grigory Smolkin wrote:


On 11/7/19 4:36 PM, Grigory Smolkin wrote:


On 11/7/19 12:56 PM, Kyotaro Horiguchi wrote:
At Thu, 7 Nov 2019 12:22:28 +0300, Grigory Smolkin 
 wrote in

On 11/7/19 8:36 AM, Kyotaro Horiguchi wrote:

At Thu, 7 Nov 2019 02:28:39 +0300, Grigory Smolkin
 wrote in

On 11/6/19 1:55 PM, Grigory Smolkin wrote:

On 11/6/19 12:56 PM, Fujii Masao wrote:
What happens if this parameter is set to latest in the standby 
mode?

Or the combination of those settings should be prohibited?

Currently it will behave just like regular standby, so I think, to
avoid confusion and keep things simple, the combination of them 
should

be prohibited.
Thank you for pointing this out, I will work on it.

Attached new patch revision, now it is impossible to use
recovery_target 'latest' in standby mode.
TAP test is updated to reflect this behavior.

In the first place, latest (or anything it could be named as) is
defined as the explit label for the default behavior. Thus the latest
should work as if nothing is set to recovery_target* following the
definition.  That might seems somewhat strange but I think at 
least it

is harmless.


Well, it was more about getting default behavior by using some
explicit recovery_target, not the other way around. Because it will
break some 3rd party backup and replication applications, that may
rely on old behavior of ignoring recovery_target_action when no
recovery_target is provided.
But you think that it is worth pursuing, I can do that.

Ah. Sorry for the misleading statement. What I had in my mind was
somewhat the mixture of them.  I thought that recovery_target =''
behaves the same way as now, r_t_action is ignored. And 'latest' just
makes recovery_target_action work as the current non-empty
recovery_target's does. But I'm not confident that it is a good
design.

recovery_target=immediate + r_t_action=shutdown for a standby 
works as

commanded. Do we need to inhibit that, too?

Why something, that work as expected, should be inhibited?

To make sure, I don't think we should do that. I meant by the above
that standby mode is already accepting recovery_target_action so
inhibiting that only for 'latest' is not orthogonal and could be more
confusing for users, and complicatig the code. So my opinion is we
shouldn't inhibit 'latest' unless r_t_action harms.


I gave it some thought and now think that prohibiting recovery_target 
'latest' and standby was a bad idea.
All recovery_targets follow the same pattern of usage, so 
recovery_target "latest" also must be capable of working in standby 
mode.
All recovery_targets have a clear deterministic 'target' where 
recovery should end.
In case of recovery_target "latest" this target is the end of 
available WAL, therefore the end of available WAL must be more 
clearly defined.

I will work on it.

Thank you for a feedback.



Attached new patch revision, now end of available WAL is defined as 
the fact of missing required WAL.
In case of standby, the end of WAL is defined as 2 consecutive 
switches of WAL source, that didn`t provided requested record.
In case of streaming standby, each switch of WAL source is forced 
after 3 failed attempts to get new data from walreceiver.


All constants are taken off the top of my head and serves as proof of 
concept.

TAP test is updated.


Attached new revision, it contains some minor refactoring.

While working on it, I stumbled upon something strange:

why DisownLatch(>recoveryWakeupLatch) is called before 
ReadRecord(xlogreader, LastRec, PANIC, false) ?


Isn`t this latch may be accessed in WaitForWALToBecomeAvailable() if 
streaming standby gets promoted?











regards.


--
Grigory Smolkin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 0191ec84b1..49675c38da 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -3366,21 +3366,19 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"'  # Windows
 
  
  
-  recovery_target = 'immediate'
+  recovery_target (string)
   
 recovery_target configuration parameter
   
   
   

-This parameter specifies that recovery should end as soon as a
-consistent state is reached, i.e. as early as possible. When restoring
-from an online backup, this means the point where taking the backup
-ended.
-   
-   
-Technically, this is a string parameter, but 'immediate'
-is currently the only allowed value.
+This parameter determines how far recovery should proceed. The value
+immediate means that recovery should end as soon as a consistent
+state is reached, i.e. as early as possible. When restoring from an online
+backup, this means the point where taking the backup ended.
+The second possible value latest means that recovery
+should proceed to the end of the 

Re: [proposal] recovery_target "latest"

[Grigory Smolkin]

On 11/7/19 4:36 PM, Grigory Smolkin wrote:


On 11/7/19 12:56 PM, Kyotaro Horiguchi wrote:
At Thu, 7 Nov 2019 12:22:28 +0300, Grigory Smolkin 
 wrote in

On 11/7/19 8:36 AM, Kyotaro Horiguchi wrote:

At Thu, 7 Nov 2019 02:28:39 +0300, Grigory Smolkin
 wrote in

On 11/6/19 1:55 PM, Grigory Smolkin wrote:

On 11/6/19 12:56 PM, Fujii Masao wrote:
What happens if this parameter is set to latest in the standby 
mode?

Or the combination of those settings should be prohibited?

Currently it will behave just like regular standby, so I think, to
avoid confusion and keep things simple, the combination of them 
should

be prohibited.
Thank you for pointing this out, I will work on it.

Attached new patch revision, now it is impossible to use
recovery_target 'latest' in standby mode.
TAP test is updated to reflect this behavior.

In the first place, latest (or anything it could be named as) is
defined as the explit label for the default behavior. Thus the latest
should work as if nothing is set to recovery_target* following the
definition.  That might seems somewhat strange but I think at least it
is harmless.


Well, it was more about getting default behavior by using some
explicit recovery_target, not the other way around. Because it will
break some 3rd party backup and replication applications, that may
rely on old behavior of ignoring recovery_target_action when no
recovery_target is provided.
But you think that it is worth pursuing, I can do that.

Ah. Sorry for the misleading statement. What I had in my mind was
somewhat the mixture of them.  I thought that recovery_target =''
behaves the same way as now, r_t_action is ignored. And 'latest' just
makes recovery_target_action work as the current non-empty
recovery_target's does. But I'm not confident that it is a good
design.


recovery_target=immediate + r_t_action=shutdown for a standby works as
commanded. Do we need to inhibit that, too?

Why something, that work as expected, should be inhibited?

To make sure, I don't think we should do that. I meant by the above
that standby mode is already accepting recovery_target_action so
inhibiting that only for 'latest' is not orthogonal and could be more
confusing for users, and complicatig the code. So my opinion is we
shouldn't inhibit 'latest' unless r_t_action harms.


I gave it some thought and now think that prohibiting recovery_target 
'latest' and standby was a bad idea.
All recovery_targets follow the same pattern of usage, so 
recovery_target "latest" also must be capable of working in standby mode.
All recovery_targets have a clear deterministic 'target' where 
recovery should end.
In case of recovery_target "latest" this target is the end of 
available WAL, therefore the end of available WAL must be more clearly 
defined.

I will work on it.

Thank you for a feedback.



Attached new patch revision, now end of available WAL is defined as the 
fact of missing required WAL.
In case of standby, the end of WAL is defined as 2 consecutive switches 
of WAL source, that didn`t provided requested record.
In case of streaming standby, each switch of WAL source is forced after 
3 failed attempts to get new data from walreceiver.


All constants are taken off the top of my head and serves as proof of 
concept.

TAP test is updated.







regards.


--
Grigory Smolkin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 619ac8c50c..f0fa0b09c8 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -3316,21 +3316,19 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"'  # Windows
 
  
  
-  recovery_target = 'immediate'
+  recovery_target (string)
   
 recovery_target configuration parameter
   
   
   

-This parameter specifies that recovery should end as soon as a
-consistent state is reached, i.e. as early as possible. When restoring
-from an online backup, this means the point where taking the backup
-ended.
-   
-   
-Technically, this is a string parameter, but 'immediate'
-is currently the only allowed value.
+This parameter determines how far recovery should proceed. The value
+immediate means that recovery should end as soon as a consistent
+state is reached, i.e. as early as possible. When restoring from an online
+backup, this means the point where taking the backup ended.
+The second possible value latest means that recovery
+should proceed to the end of the available WAL log.

   
  
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 77ad765989..74b1a6696d 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -843,6 +843,13 @@ static bool updateMinRecoveryPoint = true;
  */
 bool		reachedConsistency = false;
 
+/*
+ * Have we reached an end of 

Re: [proposal] recovery_target "latest"

[Grigory Smolkin]

On 11/7/19 12:56 PM, Kyotaro Horiguchi wrote:

At Thu, 7 Nov 2019 12:22:28 +0300, Grigory Smolkin  
wrote in

On 11/7/19 8:36 AM, Kyotaro Horiguchi wrote:

At Thu, 7 Nov 2019 02:28:39 +0300, Grigory Smolkin
 wrote in

On 11/6/19 1:55 PM, Grigory Smolkin wrote:

On 11/6/19 12:56 PM, Fujii Masao wrote:

What happens if this parameter is set to latest in the standby mode?
Or the combination of those settings should be prohibited?

Currently it will behave just like regular standby, so I think, to
avoid confusion and keep things simple, the combination of them should
be prohibited.
Thank you for pointing this out, I will work on it.

Attached new patch revision, now it is impossible to use
recovery_target 'latest' in standby mode.
TAP test is updated to reflect this behavior.

In the first place, latest (or anything it could be named as) is
defined as the explit label for the default behavior. Thus the latest
should work as if nothing is set to recovery_target* following the
definition.  That might seems somewhat strange but I think at least it
is harmless.


Well, it was more about getting default behavior by using some
explicit recovery_target, not the other way around. Because it will
break some 3rd party backup and replication applications, that may
rely on old behavior of ignoring recovery_target_action when no
recovery_target is provided.
But you think that it is worth pursuing, I can do that.

Ah. Sorry for the misleading statement. What I had in my mind was
somewhat the mixture of them.  I thought that recovery_target =''
behaves the same way as now, r_t_action is ignored. And 'latest' just
makes recovery_target_action work as the current non-empty
recovery_target's does. But I'm not confident that it is a good
design.


recovery_target=immediate + r_t_action=shutdown for a standby works as
commanded. Do we need to inhibit that, too?

Why something, that work as expected, should be inhibited?

To make sure, I don't think we should do that. I meant by the above
that standby mode is already accepting recovery_target_action so
inhibiting that only for 'latest' is not orthogonal and could be more
confusing for users, and complicatig the code. So my opinion is we
shouldn't inhibit 'latest' unless r_t_action harms.


I gave it some thought and now think that prohibiting recovery_target 
'latest' and standby was a bad idea.
All recovery_targets follow the same pattern of usage, so 
recovery_target "latest" also must be capable of working in standby mode.
All recovery_targets have a clear deterministic 'target' where recovery 
should end.
In case of recovery_target "latest" this target is the end of available 
WAL, therefore the end of available WAL must be more clearly defined.

I will work on it.

Thank you for a feedback.




regards.


--
Grigory Smolkin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [proposal] recovery_target "latest"

[Kyotaro Horiguchi]

At Thu, 7 Nov 2019 12:22:28 +0300, Grigory Smolkin  
wrote in 
> 
> On 11/7/19 8:36 AM, Kyotaro Horiguchi wrote:
> > At Thu, 7 Nov 2019 02:28:39 +0300, Grigory Smolkin
> >  wrote in
> >> On 11/6/19 1:55 PM, Grigory Smolkin wrote:
> >>> On 11/6/19 12:56 PM, Fujii Masao wrote:
>  What happens if this parameter is set to latest in the standby mode?
>  Or the combination of those settings should be prohibited?
> >>>
> >>> Currently it will behave just like regular standby, so I think, to
> >>> avoid confusion and keep things simple, the combination of them should
> >>> be prohibited.
> >>> Thank you for pointing this out, I will work on it.
> >> Attached new patch revision, now it is impossible to use
> >> recovery_target 'latest' in standby mode.
> >> TAP test is updated to reflect this behavior.
> > In the first place, latest (or anything it could be named as) is
> > defined as the explit label for the default behavior. Thus the latest
> > should work as if nothing is set to recovery_target* following the
> > definition.  That might seems somewhat strange but I think at least it
> > is harmless.
> 
> 
> Well, it was more about getting default behavior by using some
> explicit recovery_target, not the other way around. Because it will
> break some 3rd party backup and replication applications, that may
> rely on old behavior of ignoring recovery_target_action when no
> recovery_target is provided.
> But you think that it is worth pursuing, I can do that.

Ah. Sorry for the misleading statement. What I had in my mind was
somewhat the mixture of them.  I thought that recovery_target =''
behaves the same way as now, r_t_action is ignored. And 'latest' just
makes recovery_target_action work as the current non-empty
recovery_target's does. But I'm not confident that it is a good
design.

> > recovery_target=immediate + r_t_action=shutdown for a standby works as
> > commanded. Do we need to inhibit that, too?
> 
> Why something, that work as expected, should be inhibited?

To make sure, I don't think we should do that. I meant by the above
that standby mode is already accepting recovery_target_action so
inhibiting that only for 'latest' is not orthogonal and could be more
confusing for users, and complicatig the code. So my opinion is we
shouldn't inhibit 'latest' unless r_t_action harms.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: [proposal] recovery_target "latest"

[Grigory Smolkin]

On 11/7/19 8:36 AM, Kyotaro Horiguchi wrote:

At Thu, 7 Nov 2019 02:28:39 +0300, Grigory Smolkin  
wrote in

On 11/6/19 1:55 PM, Grigory Smolkin wrote:

On 11/6/19 12:56 PM, Fujii Masao wrote:

On Wed, Nov 6, 2019 at 6:33 PM Grigory Smolkin
 wrote:

On 11/6/19 10:39 AM, Peter Eisentraut wrote:

This seems to also be related to this discussion:


Yes, in a way. Strengthening current lax recovery behavior is a very
good idea.


I like this idea.

I don't like the name "latest".  What does that mean?  Other
documentation talks about the "end of the archive".  What does that
mean?  It means until restore_command errors.  Let's think of a name
that reflects that better.  Maybe "all_archive" or something like
that.

As with "immediate", "latest" reflects the latest possible state this
PostgreSQL instance can achieve when using PITR. I think it is simple
and easy to understand for an end user, which sees PITR as a way to go
from one state to another. In my experience, at least, which is, of
course, subjective.

But if we want an argument name to be technically accurate, then, I
think, something like "end-of-available-WAL"/"all-WAL", "end-of-WAL"
is
a way to go.

What happens if this parameter is set to latest in the standby mode?
Or the combination of those settings should be prohibited?


Currently it will behave just like regular standby, so I think, to
avoid confusion and keep things simple, the combination of them should
be prohibited.
Thank you for pointing this out, I will work on it.

Attached new patch revision, now it is impossible to use
recovery_target 'latest' in standby mode.
TAP test is updated to reflect this behavior.

In the first place, latest (or anything it could be named as) is
defined as the explit label for the default behavior. Thus the latest
should work as if nothing is set to recovery_target* following the
definition.  That might seems somewhat strange but I think at least it
is harmless.



Well, it was more about getting default behavior by using some explicit 
recovery_target, not the other way around. Because it will break some 
3rd party backup and replication applications, that may rely on old 
behavior of ignoring recovery_target_action when no recovery_target is 
provided.

But you think that it is worth pursuing, I can do that.



recovery_target=immediate + r_t_action=shutdown for a standby works as
commanded. Do we need to inhibit that, too?


Why something, that work as expected, should be inhibited?





The other way around, as I see it, is to define RECOVERY_TARGET_LATEST
as something more complex, for example, the latest possible endptr in
latest WAL segment. But it is tricky, because WAL archive may keeps
growing as recovery is progressing or, in case of standby, master
keeps sending more and more WAL.

regards.


--
Grigory Smolkin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [proposal] recovery_target "latest"

[Kyotaro Horiguchi]

At Thu, 7 Nov 2019 02:28:39 +0300, Grigory Smolkin  
wrote in 
> On 11/6/19 1:55 PM, Grigory Smolkin wrote:
> >
> > On 11/6/19 12:56 PM, Fujii Masao wrote:
> >> On Wed, Nov 6, 2019 at 6:33 PM Grigory Smolkin
> >>  wrote:
> >>>
> >>> On 11/6/19 10:39 AM, Peter Eisentraut wrote:
>  This seems to also be related to this discussion:
>  
> >>> Yes, in a way. Strengthening current lax recovery behavior is a very
> >>> good idea.
> >>>
>  I like this idea.
> 
>  I don't like the name "latest".  What does that mean?  Other
>  documentation talks about the "end of the archive".  What does that
>  mean?  It means until restore_command errors.  Let's think of a name
>  that reflects that better.  Maybe "all_archive" or something like
>  that.
> >>> As with "immediate", "latest" reflects the latest possible state this
> >>> PostgreSQL instance can achieve when using PITR. I think it is simple
> >>> and easy to understand for an end user, which sees PITR as a way to go
> >>> from one state to another. In my experience, at least, which is, of
> >>> course, subjective.
> >>>
> >>> But if we want an argument name to be technically accurate, then, I
> >>> think, something like "end-of-available-WAL"/"all-WAL", "end-of-WAL"
> >>> is
> >>> a way to go.
> >> What happens if this parameter is set to latest in the standby mode?
> >> Or the combination of those settings should be prohibited?
> >
> >
> > Currently it will behave just like regular standby, so I think, to
> > avoid confusion and keep things simple, the combination of them should
> > be prohibited.
> > Thank you for pointing this out, I will work on it.
> 
> Attached new patch revision, now it is impossible to use
> recovery_target 'latest' in standby mode.
> TAP test is updated to reflect this behavior.

In the first place, latest (or anything it could be named as) is
defined as the explit label for the default behavior. Thus the latest
should work as if nothing is set to recovery_target* following the
definition.  That might seems somewhat strange but I think at least it
is harmless.

recovery_target=immediate + r_t_action=shutdown for a standby works as
commanded. Do we need to inhibit that, too?

> > The other way around, as I see it, is to define RECOVERY_TARGET_LATEST
> > as something more complex, for example, the latest possible endptr in
> > latest WAL segment. But it is tricky, because WAL archive may keeps
> > growing as recovery is progressing or, in case of standby, master
> > keeps sending more and more WAL.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: [proposal] recovery_target "latest"

[Grigory Smolkin]

On 11/6/19 1:55 PM, Grigory Smolkin wrote:


On 11/6/19 12:56 PM, Fujii Masao wrote:
On Wed, Nov 6, 2019 at 6:33 PM Grigory Smolkin 
 wrote:


On 11/6/19 10:39 AM, Peter Eisentraut wrote:

This seems to also be related to this discussion:
 


Yes, in a way. Strengthening current lax recovery behavior is a very
good idea.


I like this idea.

I don't like the name "latest".  What does that mean?  Other
documentation talks about the "end of the archive".  What does that
mean?  It means until restore_command errors.  Let's think of a name
that reflects that better.  Maybe "all_archive" or something like 
that.

As with "immediate", "latest" reflects the latest possible state this
PostgreSQL instance can achieve when using PITR. I think it is simple
and easy to understand for an end user, which sees PITR as a way to go
from one state to another. In my experience, at least, which is, of
course, subjective.

But if we want an argument name to be technically accurate, then, I
think, something like "end-of-available-WAL"/"all-WAL", "end-of-WAL" is
a way to go.

What happens if this parameter is set to latest in the standby mode?
Or the combination of those settings should be prohibited?



Currently it will behave just like regular standby, so I think, to 
avoid confusion and keep things simple, the combination of them should 
be prohibited.

Thank you for pointing this out, I will work on it.


Attached new patch revision, now it is impossible to use recovery_target 
'latest' in standby mode.

TAP test is updated to reflect this behavior.




The other way around, as I see it, is to define RECOVERY_TARGET_LATEST 
as something more complex, for example, the latest possible endptr in 
latest WAL segment. But it is tricky, because WAL archive may keeps 
growing as recovery is progressing or, in case of standby, master 
keeps sending more and more WAL.




Regards,


--
Grigory Smolkin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 619ac8c50c..f0fa0b09c8 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -3316,21 +3316,19 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"'  # Windows
 
  
  
-  recovery_target = 'immediate'
+  recovery_target (string)
   
 recovery_target configuration parameter
   
   
   

-This parameter specifies that recovery should end as soon as a
-consistent state is reached, i.e. as early as possible. When restoring
-from an online backup, this means the point where taking the backup
-ended.
-   
-   
-Technically, this is a string parameter, but 'immediate'
-is currently the only allowed value.
+This parameter determines how far recovery should proceed. The value
+immediate means that recovery should end as soon as a consistent
+state is reached, i.e. as early as possible. When restoring from an online
+backup, this means the point where taking the backup ended.
+The second possible value latest means that recovery
+should proceed to the end of the available WAL log.

   
  
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 77ad765989..6001b0aa34 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -5401,6 +5401,15 @@ validateRecoveryParameters(void)
 	 errmsg("must specify restore_command when standby mode is not enabled")));
 	}
 
+	/* Combining standby mode with recovery to the end of WAL is
+	 * impossible, because there is no end of WAL in this case.
+	 */
+	if (StandbyModeRequested && recoveryTarget == RECOVERY_TARGET_LATEST)
+		ereport(FATAL,
+	(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+	 errmsg("recovery to the end of WAL is not possible because standby mode is enabled")));
+
+
 	/*
 	 * Override any inconsistent requests. Note that this is a change of
 	 * behaviour in 9.5; prior to this we simply ignored a request to pause if
@@ -6350,6 +6359,9 @@ StartupXLOG(void)
 		else if (recoveryTarget == RECOVERY_TARGET_IMMEDIATE)
 			ereport(LOG,
 	(errmsg("starting point-in-time recovery to earliest consistent point")));
+		else if (recoveryTarget == RECOVERY_TARGET_LATEST)
+			ereport(LOG,
+	(errmsg("starting point-in-time recovery until all available WAL is applied")));
 		else
 			ereport(LOG,
 	(errmsg("starting archive recovery")));
@@ -7261,6 +7273,13 @@ StartupXLOG(void)
 			 * end of main redo apply loop
 			 */
 
+			/*
+			 * If all avaialble WAL is replayed,
+			 * then RECOVERY_TARGET_LATEST is satisfied
+			 */
+			if (recoveryTarget == RECOVERY_TARGET_LATEST)
+reachedStopPoint = true;
+
 			if (reachedStopPoint)
 			{
 if (!reachedConsistency)
@@ -7462,6 +7481,8 @@ StartupXLOG(void)
 		

Re: [proposal] recovery_target "latest"

[Grigory Smolkin]

On 11/6/19 12:56 PM, Fujii Masao wrote:

On Wed, Nov 6, 2019 at 6:33 PM Grigory Smolkin  wrote:


On 11/6/19 10:39 AM, Peter Eisentraut wrote:

This seems to also be related to this discussion:


Yes, in a way. Strengthening current lax recovery behavior is a very
good idea.


I like this idea.

I don't like the name "latest".  What does that mean?  Other
documentation talks about the "end of the archive".  What does that
mean?  It means until restore_command errors.  Let's think of a name
that reflects that better.  Maybe "all_archive" or something like that.

As with "immediate", "latest" reflects the latest possible state this
PostgreSQL instance can achieve when using PITR. I think it is simple
and easy to understand for an end user, which sees PITR as a way to go
from one state to another. In my experience, at least, which is, of
course, subjective.

But if we want an argument name to be technically accurate, then, I
think, something like "end-of-available-WAL"/"all-WAL", "end-of-WAL" is
a way to go.

What happens if this parameter is set to latest in the standby mode?
Or the combination of those settings should be prohibited?



Currently it will behave just like regular standby, so I think, to avoid 
confusion and keep things simple, the combination of them should be 
prohibited.

Thank you for pointing this out, I will work on it.

The other way around, as I see it, is to define RECOVERY_TARGET_LATEST 
as something more complex, for example, the latest possible endptr in 
latest WAL segment. But it is tricky, because WAL archive may keeps 
growing as recovery is progressing or, in case of standby, master keeps 
sending more and more WAL.




Regards,


--
Grigory Smolkin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [proposal] recovery_target "latest"

[Fujii Masao]

On Wed, Nov 6, 2019 at 6:33 PM Grigory Smolkin  wrote:
>
>
> On 11/6/19 10:39 AM, Peter Eisentraut wrote:
> > This seems to also be related to this discussion:
> > 
>
> Yes, in a way. Strengthening current lax recovery behavior is a very
> good idea.
>
> >
> > I like this idea.
> >
> > I don't like the name "latest".  What does that mean?  Other
> > documentation talks about the "end of the archive".  What does that
> > mean?  It means until restore_command errors.  Let's think of a name
> > that reflects that better.  Maybe "all_archive" or something like that.
>
> As with "immediate", "latest" reflects the latest possible state this
> PostgreSQL instance can achieve when using PITR. I think it is simple
> and easy to understand for an end user, which sees PITR as a way to go
> from one state to another. In my experience, at least, which is, of
> course, subjective.
>
> But if we want an argument name to be technically accurate, then, I
> think, something like "end-of-available-WAL"/"all-WAL", "end-of-WAL" is
> a way to go.

What happens if this parameter is set to latest in the standby mode?
Or the combination of those settings should be prohibited?

Regards,

-- 
Fujii Masao

Re: [proposal] recovery_target "latest"

[Grigory Smolkin]

On 11/6/19 10:39 AM, Peter Eisentraut wrote:
This seems to also be related to this discussion: 



Yes, in a way. Strengthening current lax recovery behavior is a very 
good idea.




I like this idea.

I don't like the name "latest".  What does that mean?  Other 
documentation talks about the "end of the archive".  What does that 
mean?  It means until restore_command errors.  Let's think of a name 
that reflects that better.  Maybe "all_archive" or something like that.


As with "immediate", "latest" reflects the latest possible state this 
PostgreSQL instance can achieve when using PITR. I think it is simple 
and easy to understand for an end user, which sees PITR as a way to go 
from one state to another. In my experience, at least, which is, of 
course, subjective.


But if we want an argument name to be technically accurate, then, I 
think, something like "end-of-available-WAL"/"all-WAL", "end-of-WAL" is 
a way to go.



--
Grigory Smolkin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [proposal] recovery_target "latest"

[Peter Eisentraut]

This seems to also be related to this discussion: 



I like this idea.

I don't like the name "latest".  What does that mean?  Other 
documentation talks about the "end of the archive".  What does that 
mean?  It means until restore_command errors.  Let's think of a name 
that reflects that better.  Maybe "all_archive" or something like that.


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

Re: [proposal] recovery_target "latest"

[Grigory Smolkin]

Attached new version of a patch with TAP test.

On 11/5/19 11:51 AM, Grigory Smolkin wrote:

Thank you for you interest in this topic!

On 11/5/19 10:41 AM, Kyotaro Horiguchi wrote:

Hello.

At Mon, 4 Nov 2019 16:03:38 +0300, Grigory Smolkin 
 wrote in

Hello, hackers!

I`d like to propose a new argument for recovery_target parameter,
which will stand to recovering until all available WAL segments are
applied.

Current PostgreSQL recovery default behavior(when no recovery target
is provided) does exactly that, but there are several shortcomings:
   - without explicit recovery target standing for default behavior,
recovery_target_action is not coming to action at the end of recovery
   - with PG12 changes, the life of all backup tools became very hard,
because now recovery parameters can be set outside of single config
file(recovery.conf), so it is impossible to ensure, that default
recovery behavior, desired in some cases, will not be silently
overwritten by some recovery parameter forgotten by user.

Proposed path is very simple and solves the aforementioned problems by
introducing new argument "latest" for recovery_target parameter.

Does the tool remove or rename recovery.conf to cancel the settings?
And do you intend that the new option is used to override settings by
appending it at the end of postgresql.conf? If so, the commit
f2cbffc7a6 seems to break the assumption. PG12 rejects to start if it
finds two different kinds of recovery target settings.
Yes, previously it was possible to remove/rename old recovery.conf, 
but not anymore.
My assumption is exactly that PG should reject to start because of 
multiple recovery targets.
Failing to start is infinitely better that recovering to the wrong 
recovery target.



Old recovery behavior is still available if no recovery target is
provided. I`m not sure, whether it should it be left as it is now, or
not.

Another open question is what to do with recovery_target_inclusive if
recovery_target = "latest" is used.

Anyway inclusiveness doesn't affect to "immediate". If we had the
"latest" option, it would behave the same way.

Right, thank you.


regards.


--
Grigory Smolkin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 619ac8c50c..f0fa0b09c8 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -3316,21 +3316,19 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"'  # Windows
 
  
  
-  recovery_target = 'immediate'
+  recovery_target (string)
   
 recovery_target configuration parameter
   
   
   

-This parameter specifies that recovery should end as soon as a
-consistent state is reached, i.e. as early as possible. When restoring
-from an online backup, this means the point where taking the backup
-ended.
-   
-   
-Technically, this is a string parameter, but 'immediate'
-is currently the only allowed value.
+This parameter determines how far recovery should proceed. The value
+immediate means that recovery should end as soon as a consistent
+state is reached, i.e. as early as possible. When restoring from an online
+backup, this means the point where taking the backup ended.
+The second possible value latest means that recovery
+should proceed to the end of the available WAL log.

   
  
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 77ad765989..707ebe68a1 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6350,6 +6350,9 @@ StartupXLOG(void)
 		else if (recoveryTarget == RECOVERY_TARGET_IMMEDIATE)
 			ereport(LOG,
 	(errmsg("starting point-in-time recovery to earliest consistent point")));
+		else if (recoveryTarget == RECOVERY_TARGET_LATEST)
+			ereport(LOG,
+	(errmsg("starting point-in-time recovery until all available WAL is applied")));
 		else
 			ereport(LOG,
 	(errmsg("starting archive recovery")));
@@ -7261,6 +7264,13 @@ StartupXLOG(void)
 			 * end of main redo apply loop
 			 */
 
+			/*
+			 * If all avaialble WAL is replayed,
+			 * then RECOVERY_TARGET_LATEST is satisfied
+			 */
+			if (recoveryTarget == RECOVERY_TARGET_LATEST)
+reachedStopPoint = true;
+
 			if (reachedStopPoint)
 			{
 if (!reachedConsistency)
@@ -7462,6 +7472,8 @@ StartupXLOG(void)
 	 recoveryStopName);
 		else if (recoveryTarget == RECOVERY_TARGET_IMMEDIATE)
 			snprintf(reason, sizeof(reason), "reached consistency");
+		else if (recoveryTarget == RECOVERY_TARGET_LATEST)
+			snprintf(reason, sizeof(reason), "applied all available WAL");
 		else
 			snprintf(reason, sizeof(reason), "no recovery target specified");
 
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index f0ed326a1b..e7288a79b6 100644
--- a/src/backend/utils/misc/guc.c
+++ 

Re: [proposal] recovery_target "latest"

[Grigory Smolkin]

Thank you for you interest in this topic!

On 11/5/19 10:41 AM, Kyotaro Horiguchi wrote:

Hello.

At Mon, 4 Nov 2019 16:03:38 +0300, Grigory Smolkin  
wrote in

Hello, hackers!

I`d like to propose a new argument for recovery_target parameter,
which will stand to recovering until all available WAL segments are
applied.

Current PostgreSQL recovery default behavior(when no recovery target
is provided) does exactly that, but there are several shortcomings:
   - without explicit recovery target standing for default behavior,
recovery_target_action is not coming to action at the end of recovery
   - with PG12 changes, the life of all backup tools became very hard,
because now recovery parameters can be set outside of single config
file(recovery.conf), so it is impossible to ensure, that default
recovery behavior, desired in some cases, will not be silently
overwritten by some recovery parameter forgotten by user.

Proposed path is very simple and solves the aforementioned problems by
introducing new argument "latest" for recovery_target parameter.

Does the tool remove or rename recovery.conf to cancel the settings?
And do you intend that the new option is used to override settings by
appending it at the end of postgresql.conf? If so, the commit
f2cbffc7a6 seems to break the assumption. PG12 rejects to start if it
finds two different kinds of recovery target settings.
Yes, previously it was possible to remove/rename old recovery.conf, but 
not anymore.
My assumption is exactly that PG should reject to start because of 
multiple recovery targets.
Failing to start is infinitely better that recovering to the wrong 
recovery target.



Old recovery behavior is still available if no recovery target is
provided. I`m not sure, whether it should it be left as it is now, or
not.

Another open question is what to do with recovery_target_inclusive if
recovery_target = "latest" is used.

Anyway inclusiveness doesn't affect to "immediate". If we had the
"latest" option, it would behave the same way.

Right, thank you.


regards.


--
Grigory Smolkin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [proposal] recovery_target "latest"

[Kyotaro Horiguchi]

Hello.

At Mon, 4 Nov 2019 16:03:38 +0300, Grigory Smolkin  
wrote in 
> Hello, hackers!
> 
> I`d like to propose a new argument for recovery_target parameter,
> which will stand to recovering until all available WAL segments are
> applied.
> 
> Current PostgreSQL recovery default behavior(when no recovery target
> is provided) does exactly that, but there are several shortcomings:
>   - without explicit recovery target standing for default behavior,
> recovery_target_action is not coming to action at the end of recovery
>   - with PG12 changes, the life of all backup tools became very hard,
> because now recovery parameters can be set outside of single config
> file(recovery.conf), so it is impossible to ensure, that default
> recovery behavior, desired in some cases, will not be silently
> overwritten by some recovery parameter forgotten by user.
> 
> Proposed path is very simple and solves the aforementioned problems by
> introducing new argument "latest" for recovery_target parameter.

Does the tool remove or rename recovery.conf to cancel the settings?
And do you intend that the new option is used to override settings by
appending it at the end of postgresql.conf? If so, the commit
f2cbffc7a6 seems to break the assumption. PG12 rejects to start if it
finds two different kinds of recovery target settings.

> Old recovery behavior is still available if no recovery target is
> provided. I`m not sure, whether it should it be left as it is now, or
> not.
> 
> Another open question is what to do with recovery_target_inclusive if
> recovery_target = "latest" is used.

Anyway inclusiveness doesn't affect to "immediate". If we had the
"latest" option, it would behave the same way.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

[proposal] recovery_target "latest"

[Grigory Smolkin]

Hello, hackers!

I`d like to propose a new argument for recovery_target parameter, which 
will stand to recovering until all available WAL segments are applied.


Current PostgreSQL recovery default behavior(when no recovery target is 
provided) does exactly that, but there are several shortcomings:
  - without explicit recovery target standing for default behavior, 
recovery_target_action is not coming to action at the end of recovery
  - with PG12 changes, the life of all backup tools became very hard, 
because now recovery parameters can be set outside of single config 
file(recovery.conf), so it is impossible to ensure, that default 
recovery behavior, desired in some cases, will not be silently 
overwritten by some recovery parameter forgotten by user.


Proposed path is very simple and solves the aforementioned problems by 
introducing new argument "latest" for recovery_target parameter.


Old recovery behavior is still available if no recovery target is 
provided. I`m not sure, whether it should it be left as it is now, or not.


Another open question is what to do with recovery_target_inclusive if 
recovery_target = "latest" is used.


--
Grigory Smolkin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 0191ec84b1..49675c38da 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -3366,21 +3366,19 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"'  # Windows
 
  
  
-  recovery_target = 'immediate'
+  recovery_target (string)
   
 recovery_target configuration parameter
   
   
   

-This parameter specifies that recovery should end as soon as a
-consistent state is reached, i.e. as early as possible. When restoring
-from an online backup, this means the point where taking the backup
-ended.
-   
-   
-Technically, this is a string parameter, but 'immediate'
-is currently the only allowed value.
+This parameter determines how far recovery should proceed. The value
+immediate means that recovery should end as soon as a consistent
+state is reached, i.e. as early as possible. When restoring from an online
+backup, this means the point where taking the backup ended.
+The second possible value latest means that recovery
+should proceed to the end of the available WAL log.

   
  
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 2e3cc51006..faab0b2b4c 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6345,6 +6345,9 @@ StartupXLOG(void)
 		else if (recoveryTarget == RECOVERY_TARGET_IMMEDIATE)
 			ereport(LOG,
 	(errmsg("starting point-in-time recovery to earliest consistent point")));
+		else if (recoveryTarget == RECOVERY_TARGET_LATEST)
+			ereport(LOG,
+	(errmsg("starting point-in-time recovery until all available WAL is applied")));
 		else
 			ereport(LOG,
 	(errmsg("starting archive recovery")));
@@ -7257,6 +7260,13 @@ StartupXLOG(void)
 			 * end of main redo apply loop
 			 */
 
+			/*
+			 * If all avaialble WAL is replayed,
+			 * then RECOVERY_TARGET_LATEST is satisfied
+			 */
+			if (recoveryTarget == RECOVERY_TARGET_LATEST)
+reachedStopPoint = true;
+
 			if (reachedStopPoint)
 			{
 if (!reachedConsistency)
@@ -7459,6 +7469,8 @@ StartupXLOG(void)
 	 recoveryStopName);
 		else if (recoveryTarget == RECOVERY_TARGET_IMMEDIATE)
 			snprintf(reason, sizeof(reason), "reached consistency");
+		else if (recoveryTarget == RECOVERY_TARGET_LATEST)
+			snprintf(reason, sizeof(reason), "applied all available WAL");
 		else
 			snprintf(reason, sizeof(reason), "no recovery target specified");
 
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 31a5ef0474..b652304683 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -3503,7 +3503,10 @@ static struct config_string ConfigureNamesString[] =
 
 	{
 		{"recovery_target", PGC_POSTMASTER, WAL_RECOVERY_TARGET,
-			gettext_noop("Set to \"immediate\" to end recovery as soon as a consistent state is reached."),
+			gettext_noop("Set to \"immediate\" to end recovery as "
+		 "soon as a consistent state is reached or "
+		 " to \"latest\" to end recovery after "
+		 "replaying all available WAL in the archive."),
 			NULL
 		},
 		_target_string,
@@ -11527,9 +11530,10 @@ error_multiple_recovery_targets(void)
 static bool
 check_recovery_target(char **newval, void **extra, GucSource source)
 {
-	if (strcmp(*newval, "immediate") != 0 && strcmp(*newval, "") != 0)
+	if (strcmp(*newval, "immediate") != 0 && strcmp(*newval, "latest") != 0 &&
+		strcmp(*newval, "") != 0)
 	{
-		GUC_check_errdetail("The only allowed value is \"immediate\".");
+		GUC_check_errdetail("The only allowed values are \"immediate\"