pause recovery if pitr target not reached
This patch allows PostgreSQL to pause recovery before PITR target is reached
if recovery_target_time is specified.
Missing WAL's could then be restored from backup and applied on next restart.
Today PostgreSQL opens the database in read/write on a new timeline even when
PITR tareg is not reached.
make check is run with this patch with result "All 192 tests passed."
Source used is from version 12b4.
For both examples below "recovery_target_time = '2019-09-17 09:24:00'"
_________________________
Log from todays behavior:
[20875]: LOG: archive recovery complete cp: cannot stat '/var/lib/pgsql/12/archivedwal/00000001.history': No such file or directory
[20875]: LOG: archive recovery complete cp: cannot stat '/var/lib/pgsql/12/archivedwal/00000001.history': No such file or directory
[20875]: LOG: archive recovery complete cp: cannot stat '/var/lib/pgsql/12/archivedwal/00000001.history': No such file or directory
[20875]: LOG: archive recovery complete cp: cannot stat '/var/lib/pgsql/12/archivedwal/00000001.history': No such file or directory
[20870]: LOG: database system is ready to accept connections
[20875]: LOG: archive recovery complete cp: cannot stat '/var/lib/pgsql/12/archivedwal/00000001.history': No such file or directory
[20875]: LOG: archive recovery complete cp: cannot stat '/var/lib/pgsql/12/archivedwal/00000001.history': No such file or directory
cp: cannot stat '/var/lib/pgsql/12/archivedwal/000000010000000000000005': No such file or directory
[20875]: LOG: archive recovery complete cp: cannot stat '/var/lib/pgsql/12/archivedwal/00000001.history': No such file or directory
[20875]: LOG: archive recovery complete cp: cannot stat '/var/lib/pgsql/12/archivedwal/00000001.history': No such file or directory
[20875]: LOG: archive recovery complete cp: cannot stat '/var/lib/pgsql/12/archivedwal/00000001.history': No such file or directory
cp: cannot stat '/var/lib/pgsql/12/archivedwal/00000002.history': No such file or directory
[20875]: LOG: archive recovery complete cp: cannot stat '/var/lib/pgsql/12/archivedwal/00000001.history': No such file or directory
[20875]: LOG: archive recovery complete cp: cannot stat '/var/lib/pgsql/12/archivedwal/00000001.history': No such file or directory
cp: cannot stat '/var/lib/pgsql/12/archivedwal/00000001.history': No such file or directory
[20870]: LOG: database system is ready to accept connections
________________________
And with patched source:
[20899]: HINT: Execute pg_wal_replay_resume() to continue.
[20899]: HINT: Execute pg_wal_replay_resume() to continue.
[20899]: HINT: Execute pg_wal_replay_resume() to continue.
[20899]: HINT: Execute pg_wal_replay_resume() to continue.
[20895]: LOG: database system is ready to accept read only connections
[20899]: HINT: Execute pg_wal_replay_resume() to continue.
[20899]: HINT: Execute pg_wal_replay_resume() to continue.
cp: cannot stat '/var/lib/pgsql/12m/archivedwal/000000010000000000000005': No such file or directory
[20899]: HINT: Execute pg_wal_replay_resume() to continue.
[20899]: HINT: Execute pg_wal_replay_resume() to continue.
[20899]: HINT: Execute pg_wal_replay_resume() to continue.
[20899]: HINT: Execute pg_wal_replay_resume() to continue.
[20899]: HINT: Execute pg_wal_replay_resume() to continue.
You could restore WAL in several steps and when target is reached you get this log
[21943]: LOG: archive recovery complete cp: cannot stat '/var/lib/pgsql/12m/archivedwal/00000001.history': No such file or directory
[21943]: LOG: archive recovery complete cp: cannot stat '/var/lib/pgsql/12m/archivedwal/00000001.history': No such file or directory
[21943]: LOG: archive recovery complete cp: cannot stat '/var/lib/pgsql/12m/archivedwal/00000001.history': No such file or directory
[21943]: LOG: archive recovery complete cp: cannot stat '/var/lib/pgsql/12m/archivedwal/00000001.history': No such file or directory
[21941]: LOG: database system is ready to accept connections
[21943]: LOG: archive recovery complete cp: cannot stat '/var/lib/pgsql/12m/archivedwal/00000001.history': No such file or directory
[21943]: LOG: archive recovery complete cp: cannot stat '/var/lib/pgsql/12m/archivedwal/00000001.history': No such file or directory
[21943]: LOG: archive recovery complete cp: cannot stat '/var/lib/pgsql/12m/archivedwal/00000001.history': No such file or directory
[21943]: LOG: archive recovery complete cp: cannot stat '/var/lib/pgsql/12m/archivedwal/00000001.history': No such file or directory
Execute pg_wal_replay_resume() as hinted.
[21943]: LOG: archive recovery complete cp: cannot stat '/var/lib/pgsql/12m/archivedwal/00000001.history': No such file or directory
[21943]: LOG: archive recovery complete cp: cannot stat '/var/lib/pgsql/12m/archivedwal/00000001.history': No such file or directory
cp: cannot stat '/var/lib/pgsql/12m/archivedwal/00000002.history': No such file or directory
[21943]: LOG: archive recovery complete cp: cannot stat '/var/lib/pgsql/12m/archivedwal/00000001.history': No such file or directory
[21943]: LOG: archive recovery complete cp: cannot stat '/var/lib/pgsql/12m/archivedwal/00000001.history': No such file or directory
cp: cannot stat '/var/lib/pgsql/12m/archivedwal/00000001.history': No such file or directory
[21941]: LOG: database system is ready to accept connections
----------------
Leif Gunnar Erlandsen
Attachments:
0001-pause-recovery-if-pitr-target-not-reached.patchapplication/octet-stream; name=0001-pause-recovery-if-pitr-target-not-reached.patchDownload
*** src/backend/access/transam/xlog.c_ 2019-09-09 22:24:29.000000000 +0200
--- src/backend/access/transam/xlog.c 2019-09-17 11:54:22.631882409 +0200
***************
*** 7279,7284 ****
--- 7279,7307 ----
case RECOVERY_TARGET_ACTION_PROMOTE:
break;
}
+ } else if (recoveryTarget == RECOVERY_TARGET_TIME)
+ {
+ /*
+ * Stop point not reached and next WAL could not be read
+ * Report transaction log time and pause recovery
+ */
+ switch (recoveryTargetAction)
+ {
+ case RECOVERY_TARGET_ACTION_PAUSE:
+ ereport(LOG,
+ (errmsg("Recovery target not reached but next WAL record culd not be read")));
+ ereport(LOG,
+ (errmsg("redo done at %X/%X",
+ (uint32)(ReadRecPtr >> 32), (uint32)ReadRecPtr)));
+ xtime = GetLatestXTime();
+ if (xtime)
+ ereport(LOG,
+ (errmsg("last completed transaction was at log time %s",
+ timestamptz_to_str(xtime))));
+ SetRecoveryPause(true);
+ recoveryPausesHere();
+ break;
+ }
}
/* Allow resource managers to do any required cleanup. */
On 2019-09-17 13:23, Leif Gunnar Erlandsen wrote:
This patch allows PostgreSQL to pause recovery before PITR target is reached
if recovery_target_time is specified.Missing WAL's could then be restored from backup and applied on next restart.
Today PostgreSQL opens the database in read/write on a new timeline even when
PITR tareg is not reached.
I think this idea is worth thinking about. I don't think this should be
specific to a time-based recovery target. This could apply for example
to a target xid as well. Also, there should be a way to get the old
behavior. Perhaps this whole thing should be a new
recovery_target_action, say, 'pause_unless_reached'.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sat, 2019-10-19 at 21:45 +0200, Peter Eisentraut wrote:
On 2019-09-17 13:23, Leif Gunnar Erlandsen wrote:
This patch allows PostgreSQL to pause recovery before PITR target is reached
if recovery_target_time is specified.Missing WAL's could then be restored from backup and applied on next restart.
Today PostgreSQL opens the database in read/write on a new timeline even when
PITR tareg is not reached.I think this idea is worth thinking about. I don't think this should be
specific to a time-based recovery target. This could apply for example
to a target xid as well. Also, there should be a way to get the old
behavior. Perhaps this whole thing should be a new
recovery_target_action, say, 'pause_unless_reached'.
+1 for pausing if end-of-logs is reached before the recovery target.
I don't think that we need to add a new "recovery_target_action" to
retain the old behavior, because I think that nobody ever wants that.
I'd say that this typically happens in two cases:
1. Someone forgot to archive the WAL segment that contains the target.
In this case the proposed change will solve the problem.
2. Someone specified the recovery target wrong, e.g. used CET rather
than CEST in the recovery target time, so that the recovery target
was later than intended.
In that case the only solution is to start recovery from scratch.
But perhaps there are use cases I didn't think of.
Yours,
Laurenz Albe
On Sun, Oct 20, 2019 at 4:46 AM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
On 2019-09-17 13:23, Leif Gunnar Erlandsen wrote:
This patch allows PostgreSQL to pause recovery before PITR target is reached
if recovery_target_time is specified.Missing WAL's could then be restored from backup and applied on next restart.
Today PostgreSQL opens the database in read/write on a new timeline even when
PITR tareg is not reached.I think this idea is worth thinking about. I don't think this should be
specific to a time-based recovery target. This could apply for example
to a target xid as well. Also, there should be a way to get the old
behavior. Perhaps this whole thing should be a new
recovery_target_action, say, 'pause_unless_reached'.
Probably we can use standby mode + recovery target setting for
the almost same purpose. In this configuration, if end-of-WAL is reached
before recovery target, the startup process keeps waiting for new WAL to
be available. Then, if recovery target is reached, the startup process works
as recovery_target_action indicates.
Regards,
--
Fujii Masao
On 2019-10-21 08:44, Fujii Masao wrote:
Probably we can use standby mode + recovery target setting for
the almost same purpose. In this configuration, if end-of-WAL is reached
before recovery target, the startup process keeps waiting for new WAL to
be available. Then, if recovery target is reached, the startup process works
as recovery_target_action indicates.
So basically get rid of recovery.signal mode and honor recovery target
parameters in standby mode? That has some appeal because it simplify
this whole space significantly, but perhaps it would be too confusing
for end users?
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Nov 1, 2019 at 9:41 PM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
On 2019-10-21 08:44, Fujii Masao wrote:
Probably we can use standby mode + recovery target setting for
the almost same purpose. In this configuration, if end-of-WAL is reached
before recovery target, the startup process keeps waiting for new WAL to
be available. Then, if recovery target is reached, the startup process works
as recovery_target_action indicates.So basically get rid of recovery.signal mode and honor recovery target
parameters in standby mode?
Yes, currently not only archive recovery mode but also standby mode honors
the recovery target settings.
That has some appeal because it simplify
this whole space significantly, but perhaps it would be too confusing
for end users?
This looks less confusing than extending archive recovery. But I'd like to
hear more opinions about that.
Regards,
--
Fujii Masao
On 2019-09-17 13:23, Leif Gunnar Erlandsen wrote:
This patch allows PostgreSQL to pause recovery before PITR target is reached
if recovery_target_time is specified.
Btw., this discussion/patch seems related:
/messages/by-id/a3f650f1-fb0f-c913-a000-a4671f12a013@postgrespro.ru
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
"Peter Eisentraut" <peter.eisentraut@2ndquadrant.com> skrev 6. november 2019 kl. 08:32:
Btw., this discussion/patch seems related:
/messages/by-id/a3f650f1-fb0f-c913-a000-a4671f12a013@postgrespro.ru
I have read through this other proposal. As far as I could see in the suggested patch, it does not solve the same problem.
It still stops recovery when the recovery process does not find any more WAL.
I would like the process to pause so administrator get to choose to find more WAL to apply.
My patch should probably be extended to include
RECOVERY_TARGET_XID, RECOVERY_TARGET_NAME, RECOVERY_TARGET_LSN as well as RECOVERY_TARGET_TIME.
---
Leif Gunnar Erlandsen
After studying this a bit more, I think the current behavior is totally
bogus and needs a serious rethink.
If you specify a recovery target and it is reached, recovery pauses
(depending on recovery_target_action).
If you specify a recovery target and it is not reached when the end of
the archive is reached (i.e., restore_command fails), then recovery ends
and the server is promoted, without any further information. This is
clearly wrong in multiple ways.
I think what we should do is if we specify a recovery target and we
don't reach it, we should ereport(FATAL). Somewhere around
/*
* end of main redo apply loop
*/
in StartupXLOG(), where we already check for other conditions that are
undesirable at the end of recovery. Then a user can make fixes either
by getting more WAL files to restore and adjusting the recovery target
and starting again. I don't think pausing is the right behavior, but
perhaps an argument could be made to offer it as a nondefault behavior.
There is an interesting overlap with the other thread that wants to make
"end of archive" and explicitly settable recovery target. The current
behavior, however, is more like "recovery time (say) or end of archive,
whichever happens first", which is not a behavior that is currently
selectable or intended with other methods of recovery target
specification. Also, if you want the end of the archive as your
recovery target, that currently does not respect the
recovery_target_action setting, but perhaps it should.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Adding another patch which is not only for recovery_target_time but also for xid, name and lsn.
After studying this a bit more, I think the current behavior is totally bogus and needs a serious
rethink.If you specify a recovery target and it is reached, recovery pauses (depending on
recovery_target_action).If you specify a recovery target and it is not reached when the end of the archive is reached
(i.e., restore_command fails), then recovery ends and the server is promoted, without any further
information. This is clearly wrong in multiple ways.
Yes, that is why I have created the patch.
I think what we should do is if we specify a recovery target and we don't reach it, we should
ereport(FATAL). Somewhere around
If recovery pauses or a FATAL error is reported, is not important, as long as it is possible to get some more WAL and continue recovery. Pause has the benefit of the possibility to inspect tables in the database.
in StartupXLOG(), where we already check for other conditions that are undesirable at the end of
recovery. Then a user can make fixes either by getting more WAL files to restore and adjusting the
recovery target and starting again. I don't think pausing is the right behavior, but perhaps an
argument could be made to offer it as a nondefault behavior.
Pausing was choosen in the patch as pause was the expected behaivior if target was reached.
And the patch does not interfere with any other functionality as far as I know.
--
Leif Gunnar Erlandsen
Attachments:
0002-pause-recovery-if-pitr-target-not-reached.patchapplication/octet-stream; name=0002-pause-recovery-if-pitr-target-not-reached.patchDownload
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 2e3cc51006..566e2eb092 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -7289,6 +7289,32 @@ StartupXLOG(void)
case RECOVERY_TARGET_ACTION_PROMOTE:
break;
}
+ } else if (recoveryTarget == RECOVERY_TARGET_TIME ||
+ recoveryTarget == RECOVERY_TARGET_XID ||
+ recoveryTarget == RECOVERY_TARGET_NAME ||
+ recoveryTarget == RECOVERY_TARGET_LSN)
+ {
+ /*
+ * Stop point not reached and next WAL could not be read
+ * Report transaction log time and pause recovery
+ */
+ switch (recoveryTargetAction)
+ {
+ case RECOVERY_TARGET_ACTION_PAUSE:
+ ereport(LOG,
+ (errmsg("Recovery target not reached but next WAL record culd not be read")));
+ ereport(LOG,
+ (errmsg("redo done at %X/%X",
+ (uint32)(ReadRecPtr >> 32), (uint32)ReadRecPtr)));
+ xtime = GetLatestXTime();
+ if (xtime)
+ ereport(LOG,
+ (errmsg("last completed transaction was at log time %s",
+ timestamptz_to_str(xtime))));
+ SetRecoveryPause(true);
+ recoveryPausesHere();
+ break;
+ }
}
/* Allow resource managers to do any required cleanup. */
Hello, Lief, Peter.
At Thu, 21 Nov 2019 12:50:18 +0000, "Leif Gunnar Erlandsen" <leif@lako.no> wrote in
Adding another patch which is not only for recovery_target_time but also for xid, name and lsn.
After studying this a bit more, I think the current behavior is totally bogus and needs a serious
rethink.If you specify a recovery target and it is reached, recovery pauses (depending on
recovery_target_action).If you specify a recovery target and it is not reached when the end of the archive is reached
(i.e., restore_command fails), then recovery ends and the server is promoted, without any further
information. This is clearly wrong in multiple ways.Yes, that is why I have created the patch.
It seems premising to be used in prepeated trial-and-error recovery by
well experiecned operators. When it is used, I think that the target
goes back gradually through repetitions so anyway we need to start
from a clean backup for each repetition, in the expected
usage. Unintended promotion doesn't harm in the case.
In this persipective, I don't think the behavior is totally wrong but
FATAL'ing at EO-WAL before target seems good to do.
I think what we should do is if we specify a recovery target and we don't reach it, we should
ereport(FATAL). Somewhere aroundIf recovery pauses or a FATAL error is reported, is not important, as long as it is possible to get some more WAL and continue recovery. Pause has the benefit of the possibility to inspect tables in the database.
in StartupXLOG(), where we already check for other conditions that are undesirable at the end of
recovery. Then a user can make fixes either by getting more WAL files to restore and adjusting the
recovery target and starting again. I don't think pausing is the right behavior, but perhaps an
argument could be made to offer it as a nondefault behavior.Pausing was choosen in the patch as pause was the expected behaivior if target was reached.
And the patch does not interfere with any other functionality as far as I know.
With the current behavior, if server promotes without stopping as told
by target_action variables, it is a sign that something's wrong. But
if server pauses before reaching target, operators may overlook the
message if they don't know of the behavior. And if server poses in the
case, I think there's nothing to do.
So +1 for FATAL.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On 2019-11-21 13:50, Leif Gunnar Erlandsen wrote:
Pausing was choosen in the patch as pause was the expected behaivior if target was reached.
Pausing is the expect behavior when the target is reached because that
is the default setting of recovery_target_action. Your patch does not
take recovery_target_action into account.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
"Kyotaro Horiguchi" <horikyota.ntt@gmail.com> skrev 22. november 2019 kl. 05:26:
Hello, Lief, Peter.
At Thu, 21 Nov 2019 12:50:18 +0000, "Leif Gunnar Erlandsen" <leif@lako.no> wrote in
Adding another patch which is not only for recovery_target_time but also for xid, name and lsn.
After studying this a bit more, I think the current behavior is totally bogus and needs a serious
rethink.If you specify a recovery target and it is reached, recovery pauses (depending on
recovery_target_action).If you specify a recovery target and it is not reached when the end of the archive is reached
(i.e., restore_command fails), then recovery ends and the server is promoted, without any further
information. This is clearly wrong in multiple ways.Yes, that is why I have created the patch.
It seems premising to be used in prepeated trial-and-error recovery by
well experiecned operators. When it is used, I think that the target
goes back gradually through repetitions so anyway we need to start
from a clean backup for each repetition, in the expected
usage. Unintended promotion doesn't harm in the case.
If going back in time and gradually recover less WAL todays behaiviour is adequate.
The patch is for circumstances where for some reason you do not have all the WAL's ready at once.
In this persipective, I don't think the behavior is totally wrong but
FATAL'ing at EO-WAL before target seems good to do.I think what we should do is if we specify a recovery target and we don't reach it, we should
ereport(FATAL). Somewhere aroundIf recovery pauses or a FATAL error is reported, is not important, as long as it is possible to get
some more WAL and continue recovery. Pause has the benefit of the possibility to inspect tables in
the database.in StartupXLOG(), where we already check for other conditions that are undesirable at the end of
recovery. Then a user can make fixes either by getting more WAL files to restore and adjusting the
recovery target and starting again. I don't think pausing is the right behavior, but perhaps an
argument could be made to offer it as a nondefault behavior.Pausing was choosen in the patch as pause was the expected behaivior if target was reached.
And the patch does not interfere with any other functionality as far as I know.
With the current behavior, if server promotes without stopping as told
by target_action variables, it is a sign that something's wrong. But
if server pauses before reaching target, operators may overlook the
message if they don't know of the behavior. And if server poses in the
case, I think there's nothing to do.
Yes, that is correct. FATAL might be the correct behaiviour.
Show quoted text
So +1 for FATAL.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
"Peter Eisentraut" <peter.eisentraut@2ndquadrant.com> skrev 22. november 2019 kl. 11:50:
On 2019-11-21 13:50, Leif Gunnar Erlandsen wrote:
Pausing was choosen in the patch as pause was the expected behaivior if target was reached.
Pausing is the expect behavior when the target is reached because that is the default setting of
recovery_target_action. Your patch does not take recovery_target_action into account.
No it does not. It works well to demonstrate its purpose though.
And it might be to stop with FATAL would be more correct.
Show quoted text
-- Peter Eisentraut http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Nov 22, 2019 at 11:26:59AM +0000, Leif Gunnar Erlandsen wrote:
No it does not. It works well to demonstrate its purpose though.
And it might be to stop with FATAL would be more correct.
This is still under active discussion. Please note that the latest
patch does not apply, so a rebase would be nice to have. I have moved
the patch to next CF, waiting on author.
--
Michael
Adding patch written for 13dev from git
"Michael Paquier" <michael@paquier.xyz> skrev 1. desember 2019 kl. 03:08:
Show quoted text
On Fri, Nov 22, 2019 at 11:26:59AM +0000, Leif Gunnar Erlandsen wrote:
No it does not. It works well to demonstrate its purpose though.
And it might be to stop with FATAL would be more correct.This is still under active discussion. Please note that the latest
patch does not apply, so a rebase would be nice to have. I have moved
the patch to next CF, waiting on author.
--
Michael
Attachments:
0004-pause-recovery-if-pitr-target-not-reached.patchapplication/octet-stream; name=0004-pause-recovery-if-pitr-target-not-reached.patchDownload
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 6bc1a6b46d..c9bf48748e 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -7307,6 +7307,32 @@ StartupXLOG(void)
case RECOVERY_TARGET_ACTION_PROMOTE:
break;
}
+ } else if (recoveryTarget == RECOVERY_TARGET_TIME ||
+ recoveryTarget == RECOVERY_TARGET_XID ||
+ recoveryTarget == RECOVERY_TARGET_NAME ||
+ recoveryTarget == RECOVERY_TARGET_LSN)
+ {
+ /*
+ * Stop point not reached and next WAL could not be read
+ * Report transaction log time and pause recovery
+ */
+ switch (recoveryTargetAction)
+ {
+ case RECOVERY_TARGET_ACTION_PAUSE:
+ ereport(LOG,
+ (errmsg("Recovery target not reached but next WAL record culd not be read")));
+ ereport(LOG,
+ (errmsg("redo done at %X/%X",
+ (uint32)(ReadRecPtr >> 32), (uint32)ReadRecPtr)));
+ xtime = GetLatestXTime();
+ if (xtime)
+ ereport(LOG,
+ (errmsg("last completed transaction was at log time %s",
+ timestamptz_to_str(xtime))));
+ SetRecoveryPause(true);
+ recoveryPausesHere();
+ break;
+ }
}
/* Allow resource managers to do any required cleanup. */
On 2019-12-11 12:40, Leif Gunnar Erlandsen wrote:
Adding patch written for 13dev from git
"Michael Paquier" <michael@paquier.xyz> skrev 1. desember 2019 kl. 03:08:
On Fri, Nov 22, 2019 at 11:26:59AM +0000, Leif Gunnar Erlandsen wrote:
No it does not. It works well to demonstrate its purpose though.
And it might be to stop with FATAL would be more correct.This is still under active discussion. Please note that the latest
patch does not apply, so a rebase would be nice to have. I have moved
the patch to next CF, waiting on author.
I reworked your patch a bit. I changed the outcome to be an error, as
was discussed. I also added tests and documentation. Please take a look.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
v5-0001-Fail-if-recovery-target-is-not-reached.patchtext/plain; charset=UTF-8; name=v5-0001-Fail-if-recovery-target-is-not-reached.patch; x-mac-creator=0; x-mac-type=0Download
From 0850de3922854ecdd4249f98ea854afd6ecc9ae2 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Tue, 14 Jan 2020 21:01:26 +0100
Subject: [PATCH v5] Fail if recovery target is not reached
Before, if a recovery target is configured, but the archive ended
before the target was reached, recovery would end and the server would
promote without further notice. That was deemed to be pretty wrong.
With this change, if the recovery target is not reached, it is a fatal
error.
Discussion: https://www.postgresql.org/message-id/flat/993736dd3f1713ec1f63fc3b653839f5@lako.no
---
doc/src/sgml/config.sgml | 5 +++
src/backend/access/transam/xlog.c | 19 +++++++++
src/test/perl/PostgresNode.pm | 33 +++++++++++++--
src/test/recovery/t/003_recovery_targets.pl | 45 ++++++++++++++++++++-
4 files changed, 98 insertions(+), 4 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 5d45b6f7cb..385ccc7196 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -3571,6 +3571,11 @@ <title>Recovery Target</title>
If <xref linkend="guc-hot-standby"/> is not enabled, a setting of
<literal>pause</literal> will act the same as <literal>shutdown</literal>.
</para>
+ <para>
+ In any case, if a recovery target is configured but the archive
+ recovery ends before the target is reached, the server will shut down
+ with a fatal error.
+ </para>
</listitem>
</varlistentry>
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 7f4f784c0e..43a78e365d 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -7303,6 +7303,15 @@ StartupXLOG(void)
break;
}
}
+ else if (recoveryTarget != RECOVERY_TARGET_UNSET)
+ {
+ /*
+ * A recovery target was set but we got here without the
+ * target being reached.
+ */
+ ereport(FATAL,
+ (errmsg("recovery ended before configured recovery target was reached")));
+ }
/* Allow resource managers to do any required cleanup. */
for (rmid = 0; rmid <= RM_MAX_ID; rmid++)
@@ -7324,6 +7333,16 @@ StartupXLOG(void)
}
else
{
+ if (recoveryTarget != RECOVERY_TARGET_UNSET)
+ {
+ /*
+ * A recovery target was set but we got here without the
+ * target being reached.
+ */
+ ereport(FATAL,
+ (errmsg("recovery ended before configured recovery target was reached")));
+ }
+
/* there are no WAL records following the checkpoint */
ereport(LOG,
(errmsg("redo is not required")));
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index 2e0cf4a2f3..be44e8784f 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -653,6 +653,9 @@ Restoring WAL segments from archives using restore_command can be enabled
by passing the keyword parameter has_restoring => 1. This is disabled by
default.
+Is has_restoring is used, standby mode is used by default. To use
+recovery mode instead, pass the keyword parameter standby => 0.
+
The backup is copied, leaving the original unmodified. pg_hba.conf is
unconditionally set to enable replication connections.
@@ -669,6 +672,7 @@ sub init_from_backup
$params{has_streaming} = 0 unless defined $params{has_streaming};
$params{has_restoring} = 0 unless defined $params{has_restoring};
+ $params{standby} = 1 unless defined $params{standby};
print
"# Initializing node \"$node_name\" from backup \"$backup_name\" of node \"$root_name\"\n";
@@ -699,7 +703,7 @@ port = $port
"unix_socket_directories = '$host'");
}
$self->enable_streaming($root_node) if $params{has_streaming};
- $self->enable_restoring($root_node) if $params{has_restoring};
+ $self->enable_restoring($root_node, $params{standby}) if $params{has_restoring};
return;
}
@@ -939,7 +943,7 @@ primary_conninfo='$root_connstr'
# Internal routine to enable archive recovery command on a standby node
sub enable_restoring
{
- my ($self, $root_node) = @_;
+ my ($self, $root_node, $standby) = @_;
my $path = TestLib::perl2host($root_node->archive_dir);
my $name = $self->name;
@@ -961,7 +965,30 @@ sub enable_restoring
'postgresql.conf', qq(
restore_command = '$copy_command'
));
- $self->set_standby_mode();
+ if ($standby)
+ {
+ $self->set_standby_mode();
+ }
+ else
+ {
+ $self->set_recovery_mode();
+ }
+ return;
+}
+
+=pod
+
+=item $node->set_recovery_mode()
+
+Place recovery.signal file.
+
+=cut
+
+sub set_recovery_mode
+{
+ my ($self) = @_;
+
+ $self->append_conf('recovery.signal', '');
return;
}
diff --git a/src/test/recovery/t/003_recovery_targets.pl b/src/test/recovery/t/003_recovery_targets.pl
index d8fbd50011..5e2cf217a6 100644
--- a/src/test/recovery/t/003_recovery_targets.pl
+++ b/src/test/recovery/t/003_recovery_targets.pl
@@ -3,7 +3,8 @@
use warnings;
use PostgresNode;
use TestLib;
-use Test::More tests => 8;
+use Test::More tests => 10;
+use Time::HiRes qw(usleep);
# Create and test a standby from given backup, with a certain recovery target.
# Choose $until_lsn later than the transaction commit that causes the row
@@ -145,3 +146,45 @@ sub test_recovery_standby
my $logfile = slurp_file($node_standby->logfile());
ok($logfile =~ qr/multiple recovery targets specified/,
'multiple conflicting settings');
+
+# Check behavior when recovery ends before target is reached
+
+$node_standby = get_new_node('standby_8');
+$node_standby->init_from_backup($node_master, 'my_backup',
+ has_restoring => 1, standby => 0);
+$node_standby->append_conf('postgresql.conf',
+ "recovery_target_name = 'does_not_exist'");
+
+run_log(['pg_ctl', '-D', $node_standby->data_dir,
+ '-l', $node_standby->logfile, 'start']);
+
+# wait up to 10 seconds for postgres to terminate
+foreach my $i (0..100)
+{
+ last if ! -f $node_standby->data_dir . '/postmaster.pid';
+ usleep(100_000);
+}
+$logfile = slurp_file($node_standby->logfile());
+ok($logfile =~ qr/FATAL: recovery ended before configured recovery target was reached/,
+ 'recovery end before target reached is a fatal error');
+
+# also test the case of no redo needed (same result but different code path)
+my $node2 = get_new_node('node2');
+$node2->init(allows_streaming => 1);
+$node2->append_conf(
+ 'postgresql.conf', "recovery_target_name = 'does_not_exist'
+restore_command = 'false'
+hot_standby = off");
+$node2->append_conf('recovery.signal', '');
+
+run_log(['pg_ctl', '-D', $node2->data_dir,
+ '-l', $node2->logfile, 'start']);
+
+foreach my $i (0..100)
+{
+ last if ! -f $node2->data_dir . '/postmaster.pid';
+ usleep(100_000);
+}
+$logfile = slurp_file($node2->logfile());
+ok($logfile =~ qr/FATAL: recovery ended before configured recovery target was reached/,
+ 'recovery end before target reached is a fatal error (no redo required)');
--
2.24.1
At Tue, 14 Jan 2020 21:13:51 +0100, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote in
On 2019-12-11 12:40, Leif Gunnar Erlandsen wrote:
Adding patch written for 13dev from git
"Michael Paquier" <michael@paquier.xyz> skrev 1. desember 2019
kl. 03:08:On Fri, Nov 22, 2019 at 11:26:59AM +0000, Leif Gunnar Erlandsen wrote:
No it does not. It works well to demonstrate its purpose though.
And it might be to stop with FATAL would be more correct.This is still under active discussion. Please note that the latest
patch does not apply, so a rebase would be nice to have. I have moved
the patch to next CF, waiting on author.I reworked your patch a bit. I changed the outcome to be an error, as
was discussed. I also added tests and documentation. Please take a
look.
It doesn't show how far the last recovery actually reached. I don't
think activating resource managers harms. Don't we check the
not-reached condition *only* after the else block of the "if (record
!= NULL)" statement?
/* just have to read next record after CheckPoint */
record = ReadRecord(xlogreader, InvalidXLogRecPtr, LOG, false);
}if (record != NULL)
{
...
}
else
{
/* there are no WAL records following the checkpoint */
ereport(LOG,
(errmsg("redo is not required")));
}
+ if (recoveryTarget != RECOVERY_TARGET_UNSET && !reachedStopPoint)
..
recvoery_target_* is not cleared after startup. If a server crashed
just after the last shutdown checkpoint, any recovery_target_* setting
prevents the server from starting regardless of its value.
LOG: database system was not properly shut down; automatic recovery in progress
LOG: invalid record length at 0/9000420: wanted 24, got 0
(recovery is skipped)
FATAL: recovery ended before configured recovery target was reached
I think we should ignore the setting while crash recovery. Targeted
recovery mode is documented as a feature of archive recovery. Perhaps
ArchiveRecoveryRequested is needed in the condition.
if (ArchiveRecoveryRequested &&
recoveryTarget != RECOVERY_TARGET_UNSET && !reachedStopPoint)
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
FWIW, I restate this (perhaps) more clearly.
At Wed, 15 Jan 2020 11:02:24 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in
recvoery_target_* is not cleared after startup. If a server crashed
just after the last shutdown checkpoint, any recovery_target_* setting
prevents the server from starting regardless of its value.
recvoery_target_* is not automatically cleared after a successful
archive recovery. After that, if the server crashed just after the
last shutdown checkpoint, any recovery_target_* setting prevents the
server from starting regardless of its value.
LOG: database system was not properly shut down; automatic recovery in progress
LOG: invalid record length at 0/9000420: wanted 24, got 0(recovery is skipped)
FATAL: recovery ended before configured recovery target was reached
I think we should ignore the setting while crash recovery. Targeted
recovery mode is documented as a feature of archive recovery. Perhaps
ArchiveRecoveryRequested is needed in the condition.if (ArchiveRecoveryRequested &&
recoveryTarget != RECOVERY_TARGET_UNSET && !reachedStopPoint)
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
"Peter Eisentraut" <peter.eisentraut@2ndquadrant.com> skrev 14. januar 2020 kl. 21:13:
On 2019-12-11 12:40, Leif Gunnar Erlandsen wrote:
Adding patch written for 13dev from git
"Michael Paquier" <michael@paquier.xyz> skrev 1. desember 2019 kl. 03:08:
On Fri, Nov 22, 2019 at 11:26:59AM +0000, Leif Gunnar Erlandsen wrote:No it does not. It works well to demonstrate its purpose though.
And it might be to stop with FATAL would be more correct.This is still under active discussion. Please note that the latest
patch does not apply, so a rebase would be nice to have. I have moved
the patch to next CF, waiting on author.I reworked your patch a bit. I changed the outcome to be an error, as was discussed. I also added
tests and documentation. Please take a look.
Thank you, it was not unexpexted for the patch to be a little bit smaller.
Although it would have been nice to log where recover ended before reporting fatal error.
And since you use RECOVERY_TARGET_UNSET, RECOVERY_TARGET_IMMEDIATE also gets included, is this correct?
Show quoted text
-- Peter Eisentraut http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2020-01-15 05:02, Kyotaro Horiguchi wrote:
FWIW, I restate this (perhaps) more clearly.
At Wed, 15 Jan 2020 11:02:24 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in
recvoery_target_* is not cleared after startup. If a server crashed
just after the last shutdown checkpoint, any recovery_target_* setting
prevents the server from starting regardless of its value.recvoery_target_* is not automatically cleared after a successful
archive recovery. After that, if the server crashed just after the
last shutdown checkpoint, any recovery_target_* setting prevents the
server from starting regardless of its value.
Thank you for this clarification. Here is a new patch that addresses
that and also the other comments raised about my previous patch.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
v6-0001-Fail-if-recovery-target-is-not-reached.patchtext/plain; charset=UTF-8; name=v6-0001-Fail-if-recovery-target-is-not-reached.patch; x-mac-creator=0; x-mac-type=0Download
From a0ef72e61a31d6519ead6f4d9fb9efe2a2c94990 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 27 Jan 2020 12:14:36 +0100
Subject: [PATCH v6] Fail if recovery target is not reached
Before, if a recovery target is configured, but the archive ended
before the target was reached, recovery would end and the server would
promote without further notice. That was deemed to be pretty wrong.
With this change, if the recovery target is not reached, it is a fatal
error.
Discussion: https://www.postgresql.org/message-id/flat/993736dd3f1713ec1f63fc3b653839f5@lako.no
---
doc/src/sgml/config.sgml | 5 ++++
src/backend/access/transam/xlog.c | 19 +++++++++---
src/test/perl/PostgresNode.pm | 33 +++++++++++++++++++--
src/test/recovery/t/003_recovery_targets.pl | 24 ++++++++++++++-
4 files changed, 73 insertions(+), 8 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index e07dc01e80..c1128f89ec 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -3571,6 +3571,11 @@ <title>Recovery Target</title>
If <xref linkend="guc-hot-standby"/> is not enabled, a setting of
<literal>pause</literal> will act the same as <literal>shutdown</literal>.
</para>
+ <para>
+ In any case, if a recovery target is configured but the archive
+ recovery ends before the target is reached, the server will shut down
+ with a fatal error.
+ </para>
</listitem>
</varlistentry>
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 882d5e8a73..be4c923ab1 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6200,7 +6200,7 @@ StartupXLOG(void)
XLogCtlInsert *Insert;
CheckPoint checkPoint;
bool wasShutdown;
- bool reachedStopPoint = false;
+ bool reachedRecoveryTarget = false;
bool haveBackupLabel = false;
bool haveTblspcMap = false;
XLogRecPtr RecPtr,
@@ -7103,7 +7103,7 @@ StartupXLOG(void)
*/
if (recoveryStopsBefore(xlogreader))
{
- reachedStopPoint = true; /* see below */
+ reachedRecoveryTarget = true;
break;
}
@@ -7258,7 +7258,7 @@ StartupXLOG(void)
/* Exit loop if we reached inclusive recovery target */
if (recoveryStopsAfter(xlogreader))
{
- reachedStopPoint = true;
+ reachedRecoveryTarget = true;
break;
}
@@ -7270,7 +7270,7 @@ StartupXLOG(void)
* end of main redo apply loop
*/
- if (reachedStopPoint)
+ if (reachedRecoveryTarget)
{
if (!reachedConsistency)
ereport(FATAL,
@@ -7327,7 +7327,18 @@ StartupXLOG(void)
/* there are no WAL records following the checkpoint */
ereport(LOG,
(errmsg("redo is not required")));
+
}
+
+ /*
+ * This check is intentionally after the above log messages that
+ * indicate how far recovery went.
+ */
+ if (ArchiveRecoveryRequested &&
+ recoveryTarget != RECOVERY_TARGET_UNSET &&
+ !reachedRecoveryTarget)
+ ereport(FATAL,
+ (errmsg("recovery ended before configured recovery target was reached")));
}
/*
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index 2e0cf4a2f3..be44e8784f 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -653,6 +653,9 @@ Restoring WAL segments from archives using restore_command can be enabled
by passing the keyword parameter has_restoring => 1. This is disabled by
default.
+Is has_restoring is used, standby mode is used by default. To use
+recovery mode instead, pass the keyword parameter standby => 0.
+
The backup is copied, leaving the original unmodified. pg_hba.conf is
unconditionally set to enable replication connections.
@@ -669,6 +672,7 @@ sub init_from_backup
$params{has_streaming} = 0 unless defined $params{has_streaming};
$params{has_restoring} = 0 unless defined $params{has_restoring};
+ $params{standby} = 1 unless defined $params{standby};
print
"# Initializing node \"$node_name\" from backup \"$backup_name\" of node \"$root_name\"\n";
@@ -699,7 +703,7 @@ port = $port
"unix_socket_directories = '$host'");
}
$self->enable_streaming($root_node) if $params{has_streaming};
- $self->enable_restoring($root_node) if $params{has_restoring};
+ $self->enable_restoring($root_node, $params{standby}) if $params{has_restoring};
return;
}
@@ -939,7 +943,7 @@ primary_conninfo='$root_connstr'
# Internal routine to enable archive recovery command on a standby node
sub enable_restoring
{
- my ($self, $root_node) = @_;
+ my ($self, $root_node, $standby) = @_;
my $path = TestLib::perl2host($root_node->archive_dir);
my $name = $self->name;
@@ -961,7 +965,30 @@ sub enable_restoring
'postgresql.conf', qq(
restore_command = '$copy_command'
));
- $self->set_standby_mode();
+ if ($standby)
+ {
+ $self->set_standby_mode();
+ }
+ else
+ {
+ $self->set_recovery_mode();
+ }
+ return;
+}
+
+=pod
+
+=item $node->set_recovery_mode()
+
+Place recovery.signal file.
+
+=cut
+
+sub set_recovery_mode
+{
+ my ($self) = @_;
+
+ $self->append_conf('recovery.signal', '');
return;
}
diff --git a/src/test/recovery/t/003_recovery_targets.pl b/src/test/recovery/t/003_recovery_targets.pl
index d8fbd50011..fd14bab208 100644
--- a/src/test/recovery/t/003_recovery_targets.pl
+++ b/src/test/recovery/t/003_recovery_targets.pl
@@ -3,7 +3,8 @@
use warnings;
use PostgresNode;
use TestLib;
-use Test::More tests => 8;
+use Test::More tests => 9;
+use Time::HiRes qw(usleep);
# Create and test a standby from given backup, with a certain recovery target.
# Choose $until_lsn later than the transaction commit that causes the row
@@ -145,3 +146,24 @@ sub test_recovery_standby
my $logfile = slurp_file($node_standby->logfile());
ok($logfile =~ qr/multiple recovery targets specified/,
'multiple conflicting settings');
+
+# Check behavior when recovery ends before target is reached
+
+$node_standby = get_new_node('standby_8');
+$node_standby->init_from_backup($node_master, 'my_backup',
+ has_restoring => 1, standby => 0);
+$node_standby->append_conf('postgresql.conf',
+ "recovery_target_name = 'does_not_exist'");
+
+run_log(['pg_ctl', '-D', $node_standby->data_dir,
+ '-l', $node_standby->logfile, 'start']);
+
+# wait up to 10 seconds for postgres to terminate
+foreach my $i (0..100)
+{
+ last if ! -f $node_standby->data_dir . '/postmaster.pid';
+ usleep(100_000);
+}
+$logfile = slurp_file($node_standby->logfile());
+ok($logfile =~ qr/FATAL: recovery ended before configured recovery target was reached/,
+ 'recovery end before target reached is a fatal error');
base-commit: 3e4818e9dd5be294d97ca67012528cb1c0b0ccaa
--
2.25.0
Hello.
At Mon, 27 Jan 2020 12:16:02 +0100, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote in
On 2020-01-15 05:02, Kyotaro Horiguchi wrote:
FWIW, I restate this (perhaps) more clearly.
At Wed, 15 Jan 2020 11:02:24 +0900 (JST), Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote inrecvoery_target_* is not cleared after startup. If a server crashed
just after the last shutdown checkpoint, any recovery_target_* setting
prevents the server from starting regardless of its value.recvoery_target_* is not automatically cleared after a successful
archive recovery. After that, if the server crashed just after the
last shutdown checkpoint, any recovery_target_* setting prevents the
server from starting regardless of its value.Thank you for this clarification. Here is a new patch that addresses
that and also the other comments raised about my previous patch.
The code looks fine, renaming reachedStopPoint to
reachedRecoveryTarget looks very nice. Doc part looks fine, too.
PostgresNode.pm
+Is has_restoring is used, standby mode is used by default. To use
"Is has_restoring used,", or "If has_restoring is used," ?
+ $params{standby} = 1 unless defined $params{standby};
..
- $self->enable_restoring($root_node) if $params{has_restoring};
+ $self->enable_restoring($root_node, $params{standby}) if $params{has_restoring};
...
+ if ($standby)
+ {
+ $self->set_standby_mode();
+ }
+ else
+ {
+ $self->set_recovery_mode();
+ }The change seems aiming not to break compatibility with external test
scripts, but it looks quite confusing to me. The problem here is both
enable_streaming/restoring independently put trigger files, so don't
we separte placing of trigger files out of the functions?
As you know, set_standby_mode and set_recovery_mode are described as
"internal" but actually used by some test scripts. I think it's
preferable that the functions are added in POD rather than change the
callers not to used them.
The attached patch on top of yours does that. It might be too much but
what do you think about that?
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachments:
0001-Refactor-init_from_backup.patchtext/x-patch; charset=us-asciiDownload
From 23fc36a84bf0f4bfce158b00399a5caa85bcc55f Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Tue, 28 Jan 2020 13:10:43 +0900
Subject: [PATCH] Refactor init_from_backup
To allow recovery mode in init_from_backup, refactor how parameters
are handled. On the way doing that, separate placing trigger files out
of enable_streaming and enable_restoring so that the function can
place trigger files properly.
Several tests using the internal functions enable_restoring and
enable_streaming so expose them in POD rather than change the callers
not to use the functions.
---
src/test/perl/PostgresNode.pm | 127 ++++++++++++++++----
src/test/recovery/t/003_recovery_targets.pl | 2 +-
2 files changed, 107 insertions(+), 22 deletions(-)
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index be44e8784f..7d10658eb0 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -653,8 +653,10 @@ Restoring WAL segments from archives using restore_command can be enabled
by passing the keyword parameter has_restoring => 1. This is disabled by
default.
-Is has_restoring is used, standby mode is used by default. To use
-recovery mode instead, pass the keyword parameter standby => 0.
+If has_restoring or has_restoring is used, standby mode is used by
+default. To use recovery mode instead, pass the keyword parameter
+recovery => 1. Not to use recovery nor standby mode, pass the keyword
+parameter standby => 0.
The backup is copied, leaving the original unmodified. pg_hba.conf is
unconditionally set to enable replication connections.
@@ -669,10 +671,30 @@ sub init_from_backup
my $port = $self->port;
my $node_name = $self->name;
my $root_name = $root_node->name;
+ my $connstr = $root_node->connstr;
+ my $streaming = 0;
+ my $restoring = 0;
+ my $standby = $params{standby};
+ my $recovery = $params{recovery};
- $params{has_streaming} = 0 unless defined $params{has_streaming};
- $params{has_restoring} = 0 unless defined $params{has_restoring};
- $params{standby} = 1 unless defined $params{standby};
+ $streaming = 1 if ($params{has_streaming});
+ $restoring = 1 if ($params{has_restoring});
+
+ if ($recovery)
+ {
+ croak "### Standby mode is mutually exclusive with recovery mode.\n"
+ if ($standby);
+ }
+ elsif (defined $standby)
+ {
+ # follow explict instruction
+ $standby = $params{standby};
+ }
+ elsif ($streaming || $restoring)
+ {
+ # historically both streaming and restoring turn on standby mode
+ $standby = 1;
+ }
print
"# Initializing node \"$node_name\" from backup \"$backup_name\" of node \"$root_name\"\n";
@@ -702,8 +724,31 @@ port = $port
$self->append_conf('postgresql.conf',
"unix_socket_directories = '$host'");
}
- $self->enable_streaming($root_node) if $params{has_streaming};
- $self->enable_restoring($root_node, $params{standby}) if $params{has_restoring};
+
+ if ($restoring)
+ {
+ print "### Enabling WAL restore for node \"$node_name\"\n";
+ $self->setup_restore_command($root_node);
+ }
+
+ if ($streaming)
+ {
+ print "### Enabling primary conninfo for node \"$node_name\"\n";
+ $self->setup_primary_conninfo($root_node);
+ }
+
+ # recovery mode supercedes standby mode
+ if ($recovery)
+ {
+ print "### Enabling recovery mode for node \"$node_name\"\n";
+ $self->set_recovery_mode();
+ }
+ elsif ($standby)
+ {
+ print "### Enabling standby mode for node \"$node_name\"\n";
+ $self->set_standby_mode();
+ }
+
return;
}
@@ -924,7 +969,13 @@ sub logrotate
return;
}
-# Internal routine to enable streaming replication on a standby node.
+=pod
+
+=item PostgresNode->enable_streaming(root_node)
+
+Set up primary_conninfo according to root_node then turns on standby mode.
+=cut
+
sub enable_streaming
{
my ($self, $root_node) = @_;
@@ -932,15 +983,34 @@ sub enable_streaming
my $name = $self->name;
print "### Enabling streaming replication for node \"$name\"\n";
- $self->append_conf(
- 'postgresql.conf', qq(
-primary_conninfo='$root_connstr'
-));
+ $self->setup_primary_conninfo($root_node);
$self->set_standby_mode();
return;
}
-# Internal routine to enable archive recovery command on a standby node
+# Internal routine to set up primary_conninfo
+sub setup_primary_conninfo
+{
+ my ($self, $root_node) = @_;
+ my $root_connstr = $root_node->connstr;
+ my $name = $self->name;
+
+ $self->append_conf(
+ 'postgresql.conf', qq(
+primary_conninfo='$root_connstr'
+));
+ return;
+}
+
+=pod
+
+=item PostgresNode->enable_restoring(root_node [, standby])
+
+Set up restore_command according to root_node. Also turns on standby
+mode by default. To use recovery mode instead, pass the keyword
+parameter standby => 0.
+
+=cut
sub enable_restoring
{
my ($self, $root_node, $standby) = @_;
@@ -949,6 +1019,29 @@ sub enable_restoring
print "### Enabling WAL restore for node \"$name\"\n";
+ $self->setup_restore_command($root_node);
+
+ if (!defined $standby || $standby)
+ {
+ print "### Enabling standby mode for node \"$name\"\n";
+ $self->set_standby_mode();
+ }
+ else
+ {
+ print "### Enabling recovery mode for node \"$name\"\n";
+ $self->set_recovery_mode();
+ }
+
+ return;
+}
+
+# Internal routine to set up restore_command
+sub setup_restore_command
+{
+ my ($self, $root_node, $standby) = @_;
+ my $path = TestLib::perl2host($root_node->archive_dir);
+ my $name = $self->name;
+
# On Windows, the path specified in the restore command needs to use
# double back-slashes to work properly and to be able to detect properly
# the file targeted by the copy command, so the directory value used
@@ -965,14 +1058,6 @@ sub enable_restoring
'postgresql.conf', qq(
restore_command = '$copy_command'
));
- if ($standby)
- {
- $self->set_standby_mode();
- }
- else
- {
- $self->set_recovery_mode();
- }
return;
}
diff --git a/src/test/recovery/t/003_recovery_targets.pl b/src/test/recovery/t/003_recovery_targets.pl
index fd14bab208..c76e060d72 100644
--- a/src/test/recovery/t/003_recovery_targets.pl
+++ b/src/test/recovery/t/003_recovery_targets.pl
@@ -151,7 +151,7 @@ ok($logfile =~ qr/multiple recovery targets specified/,
$node_standby = get_new_node('standby_8');
$node_standby->init_from_backup($node_master, 'my_backup',
- has_restoring => 1, standby => 0);
+ has_restoring => 1, recovery => 1);
$node_standby->append_conf('postgresql.conf',
"recovery_target_name = 'does_not_exist'");
--
2.18.2
Great job with the patch Peter, it has been even cleaner than before after you moved the check.
Show quoted text
"Peter Eisentraut" <peter.eisentraut@2ndquadrant.com> skrev 27. januar 2020 kl. 12:16:
On 2020-01-28 06:01, Kyotaro Horiguchi wrote:
Hello.
At Mon, 27 Jan 2020 12:16:02 +0100, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote in
On 2020-01-15 05:02, Kyotaro Horiguchi wrote:
FWIW, I restate this (perhaps) more clearly.
At Wed, 15 Jan 2020 11:02:24 +0900 (JST), Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote inrecvoery_target_* is not cleared after startup. If a server crashed
just after the last shutdown checkpoint, any recovery_target_* setting
prevents the server from starting regardless of its value.recvoery_target_* is not automatically cleared after a successful
archive recovery. After that, if the server crashed just after the
last shutdown checkpoint, any recovery_target_* setting prevents the
server from starting regardless of its value.Thank you for this clarification. Here is a new patch that addresses
that and also the other comments raised about my previous patch.The code looks fine, renaming reachedStopPoint to
reachedRecoveryTarget looks very nice. Doc part looks fine, too.PostgresNode.pm
+Is has_restoring is used, standby mode is used by default. To use"Is has_restoring used,", or "If has_restoring is used," ?
Committed with that fix.
The change seems aiming not to break compatibility with external test
scripts, but it looks quite confusing to me. The problem here is both
enable_streaming/restoring independently put trigger files, so don't
we separte placing of trigger files out of the functions?
Yeah, this is all historically grown, but a major refactoring seems out
of scope for this thread. It seems hard to come up with a more elegant
way, since after all the underlying mechanisms are also all intertwined.
Your patch adds even more code, so I'm not sure it's an improvement.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
At Wed, 29 Jan 2020 16:01:46 +0100, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote in
On 2020-01-28 06:01, Kyotaro Horiguchi wrote:
The code looks fine, renaming reachedStopPoint to
reachedRecoveryTarget looks very nice. Doc part looks fine, too.
PostgresNode.pm
+Is has_restoring is used, standby mode is used by default. To use
"Is has_restoring used,", or "If has_restoring is used," ?Committed with that fix.
Thanks.
The change seems aiming not to break compatibility with external test
scripts, but it looks quite confusing to me. The problem here is both
enable_streaming/restoring independently put trigger files, so don't
we separte placing of trigger files out of the functions?Yeah, this is all historically grown, but a major refactoring seems
out of scope for this thread. It seems hard to come up with a more
elegant way, since after all the underlying mechanisms are also all
intertwined. Your patch adds even more code, so I'm not sure it's an
improvement.
Yeah, as I wrote, I thogut that as too much, but I think at least POD
part for the internal-but-externally-used routines would be
needed. Don't we?
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center