On Fri, Feb 12, 2021 at 3:26 AM Robert Haas <robertmh...@gmail.com> wrote:
>
> On Thu, Feb 11, 2021 at 6:07 AM Dilip Kumar <dilipbal...@gmail.com> wrote:
> > > Thanks for the patch. I tested the new function and it works as
> > > expected. I have no further comments on the v13 patch.
> >
> > Thanks for the review and testing.
>
> I don't see a whole lot wrong with this patch, but I think there are
> some things that could make it a little clearer:
Thanks for the review
> - I suggest renaming CheckAndSetRecoveryPause() to ConfirmRecoveryPaused().
Yeah that make more sense so changed.
> - I suggest moving the definition of that function to just after
> SetRecoveryPause().
Done
> - I suggest changing the argument to SetRecoveryPause() back to bool.
> In the one place where you call SetRecoveryPause(RECOVERY_PAUSED),
> just call SetRecoveryPause(true) and ConfirmRecoveryPaused() back to
> back.
Yeah done that way, I think only in once place we were doing
SetRecoveryPause(RECOVERY_PAUSED), but after putting more thought I
think that was not required because right after setting that we are
having the while loop under that we have to call
ConfirmRecoveryPaused. So I have changed that also as
SetRecoveryPause(true) without immediate call of
ConfirmRecoveryPaused.
This in turn means that the "if" statement in
> SetRecoveryPaused() can be rewritten as if (!recoveryPaused)
> XLogCtl->recoveryPauseState = RECOVERY_NOT_PAUSED else if
> (XLogCtl->recoveryPauseState == RECOVERY_NOT_PAUSED)
> XLogCtl->recoveryPauseState = RECOVERY_PAUSE_REQUESTED(). This is
> slightly less efficient, but I don't think it matters, and I think it
> will be a lot more clear what's the job of SetRecoveryPause (say
> whether we're trying to pause or not) and what's the job of
> ConfirmRecoveryPaused (say whether we've succeeded in pausing).
Done
> - Since the numeric values of RecoveryPauseState don't matter and the
> values are never visible to anything outside the server nor stored on
> disk, I would be inclined to (a) not specify particular values in
> xlog.h and (b) remove the test-and-elog in SetRecoveryPause().
Done
> - In the places where you say:
>
> - if (((volatile XLogCtlData *) XLogCtl)->recoveryPause)
> + if (((volatile XLogCtlData *) XLogCtl)->recoveryPauseState ==
> + RECOVERY_PAUSE_REQUESTED)
>
> ...I would suggest instead testing for != RECOVERY_NOT_PAUSED. Perhaps
> we don't think RECOVERY_PAUSED can happen here. But if somehow it did,
> calling recoveryPausesHere() would be right.
Done
> There might be some more to say here, but those are things I notice on
> a first read-through.
Okay.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
From cf45d795946c0f33f7f29b533f175139e2a92583 Mon Sep 17 00:00:00 2001
From: Dilip Kumar <dilipkumar@localhost.localdomain>
Date: Wed, 27 Jan 2021 16:46:04 +0530
Subject: [PATCH v14] Provide a new interface to get the recovery pause status
Currently, pg_is_wal_replay_paused, just checks whether the recovery
pause is requested or not but it doesn't actually tell whether the
recovery is actually paused or not. So basically there is no way for
the user to know the actual status of the pause request. This patch
provides a new interface pg_get_wal_replay_pause_state that will
return the actual status of the recovery pause i.e.'not paused' if
pause is not requested, 'pause requested' if pause is requested but
recovery is not yet paused and 'paused' if recovery is actually paused.
---
doc/src/sgml/func.sgml | 32 +++++++++++---
src/backend/access/transam/xlog.c | 81 ++++++++++++++++++++++++++++------
src/backend/access/transam/xlogfuncs.c | 46 ++++++++++++++++++-
src/include/access/xlog.h | 10 ++++-
src/include/catalog/pg_proc.dat | 4 ++
5 files changed, 151 insertions(+), 22 deletions(-)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index d822427..500bc9b 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -25285,7 +25285,24 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup());
<returnvalue>boolean</returnvalue>
</para>
<para>
- Returns true if recovery is paused.
+ Returns true if recovery pause is requested.
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role="func_table_entry"><para role="func_signature">
+ <indexterm>
+ <primary>pg_get_wal_replay_pause_state</primary>
+ </indexterm>
+ <function>pg_get_wal_replay_pause_state</function> ()
+ <returnvalue>text</returnvalue>
+ </para>
+ <para>
+ Returns recovery pause state. The return values are <literal>
+ not paused</literal> if pause is not requested, <literal>
+ pause requested</literal> if pause is requested but recovery is
+ not yet paused and, <literal>paused</literal> if the recovery is
+ actually paused.
</para></entry>
</row>
@@ -25324,10 +25341,15 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup());
<returnvalue>void</returnvalue>
</para>
<para>
- Pauses recovery. While recovery is paused, no further database
- changes are applied. If hot standby is active, all new queries will
- see the same consistent snapshot of the database, and no further query
- conflicts will be generated until recovery is resumed.
+ Request to pause recovery. A request doesn't mean that recovery stops
+ right away. If you want a guarantee that recovery is actually paused,
+ you need to check for the recovery pause state returned by
+ <function>pg_get_wal_replay_pause_state()</function>. Note that
+ <function>pg_is_wal_replay_paused()</function> returns whether a request
+ is made. While recovery is paused, no further database changes are applied.
+ If hot standby is active, all new queries will see the same consistent
+ snapshot of the database, and no further query conflicts will be generated
+ until recovery is resumed.
</para>
<para>
This function is restricted to superusers by default, but other users
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index e0c37f7..4c82264 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -721,8 +721,8 @@ typedef struct XLogCtlData
* only relevant for replication or archive recovery
*/
TimestampTz currentChunkStartTime;
- /* Are we requested to pause recovery? */
- bool recoveryPause;
+ /* Recovery pause state */
+ RecoveryPauseState recoveryPauseState;
/*
* lastFpwDisableRecPtr points to the start of the last replayed
@@ -894,6 +894,7 @@ static void validateRecoveryParameters(void);
static void exitArchiveRecovery(TimeLineID endTLI, XLogRecPtr endOfLog);
static bool recoveryStopsBefore(XLogReaderState *record);
static bool recoveryStopsAfter(XLogReaderState *record);
+static void ConfirmRecoveryPaused(void);
static void recoveryPausesHere(bool endOfRecovery);
static bool recoveryApplyDelay(XLogReaderState *record);
static void SetLatestXTime(TimestampTz xtime);
@@ -6019,7 +6020,7 @@ recoveryStopsAfter(XLogReaderState *record)
}
/*
- * Wait until shared recoveryPause flag is cleared.
+ * Wait until shared recoveryPauseState is set to RECOVERY_NOT_PAUSED.
*
* endOfRecovery is true if the recovery target is reached and
* the paused state starts at the end of recovery because of
@@ -6049,34 +6050,72 @@ recoveryPausesHere(bool endOfRecovery)
(errmsg("recovery has paused"),
errhint("Execute pg_wal_replay_resume() to continue.")));
- while (RecoveryIsPaused())
+ /* loop until recoveryPauseState is set to RECOVERY_NOT_PAUSED */
+ while (GetRecoveryPauseState() != RECOVERY_NOT_PAUSED)
{
HandleStartupProcInterrupts();
if (CheckForStandbyTrigger())
return;
pgstat_report_wait_start(WAIT_EVENT_RECOVERY_PAUSE);
+
+ /*
+ * If recovery pause is requested then set it paused. While we are in
+ * the loop, user might resume and pause again so set this every time.
+ */
+ ConfirmRecoveryPaused();
+
pg_usleep(1000000L); /* 1000 ms */
pgstat_report_wait_end();
}
}
-bool
-RecoveryIsPaused(void)
+/*
+ * Get the current state of the recovery pause request.
+ */
+RecoveryPauseState
+GetRecoveryPauseState(void)
{
- bool recoveryPause;
+ RecoveryPauseState state;
SpinLockAcquire(&XLogCtl->info_lck);
- recoveryPause = XLogCtl->recoveryPause;
+ state = XLogCtl->recoveryPauseState;
SpinLockRelease(&XLogCtl->info_lck);
- return recoveryPause;
+ return state;
}
+/*
+ * Set the recovery pause state.
+ *
+ * If recovery pause is requested then sets the recovery pause state to
+ * 'pause requested' if it is not already 'paused'. Otherwise, sets it
+ * to 'not paused' to resume the recovery. The recovery pause will be
+ * confirmed by the ConfirmRecoveryPaused.
+ */
void
SetRecoveryPause(bool recoveryPause)
{
SpinLockAcquire(&XLogCtl->info_lck);
- XLogCtl->recoveryPause = recoveryPause;
+
+ if (!recoveryPause)
+ XLogCtl->recoveryPauseState = RECOVERY_NOT_PAUSED;
+ else if (XLogCtl->recoveryPauseState == RECOVERY_NOT_PAUSED)
+ XLogCtl->recoveryPauseState = RECOVERY_PAUSE_REQUESTED;
+
+ SpinLockRelease(&XLogCtl->info_lck);
+}
+
+/*
+ * Confirm the recovery pause by setting the recovery pause state to
+ * RECOVERY_PAUSED.
+ */
+static void
+ConfirmRecoveryPaused(void)
+{
+ /* If recovery pause is requested then set it paused */
+ SpinLockAcquire(&XLogCtl->info_lck);
+ if (XLogCtl->recoveryPauseState == RECOVERY_PAUSE_REQUESTED)
+ XLogCtl->recoveryPauseState = RECOVERY_PAUSED;
SpinLockRelease(&XLogCtl->info_lck);
}
@@ -6277,7 +6316,7 @@ RecoveryRequiresIntParameter(const char *param_name, int currValue, int minValue
errdetail("If recovery is unpaused, the server will shut down."),
errhint("You can then restart the server after making the necessary configuration changes.")));
- while (RecoveryIsPaused())
+ while (GetRecoveryPauseState() != RECOVERY_NOT_PAUSED)
{
HandleStartupProcInterrupts();
@@ -6296,6 +6335,13 @@ RecoveryRequiresIntParameter(const char *param_name, int currValue, int minValue
warned_for_promote = true;
}
+ /*
+ * If recovery pause is requested then set it paused. While we
+ * are in the loop, user might resume and pause again so set
+ * this every time.
+ */
+ ConfirmRecoveryPaused();
+
pgstat_report_wait_start(WAIT_EVENT_RECOVERY_PAUSE);
pg_usleep(1000000L); /* 1000 ms */
pgstat_report_wait_end();
@@ -7194,7 +7240,7 @@ StartupXLOG(void)
XLogCtl->lastReplayedTLI = XLogCtl->replayEndTLI;
XLogCtl->recoveryLastXTime = 0;
XLogCtl->currentChunkStartTime = 0;
- XLogCtl->recoveryPause = false;
+ XLogCtl->recoveryPauseState = RECOVERY_NOT_PAUSED;
SpinLockRelease(&XLogCtl->info_lck);
/* Also ensure XLogReceiptTime has a sane value */
@@ -7298,7 +7344,8 @@ StartupXLOG(void)
* otherwise would is a minor issue, so it doesn't seem worth
* adding another spinlock cycle to prevent that.
*/
- if (((volatile XLogCtlData *) XLogCtl)->recoveryPause)
+ if (((volatile XLogCtlData *) XLogCtl)->recoveryPauseState !=
+ RECOVERY_NOT_PAUSED)
recoveryPausesHere(false);
/*
@@ -7323,7 +7370,8 @@ StartupXLOG(void)
* here otherwise pausing during the delay-wait wouldn't
* work.
*/
- if (((volatile XLogCtlData *) XLogCtl)->recoveryPause)
+ if (((volatile XLogCtlData *) XLogCtl)->recoveryPauseState !=
+ RECOVERY_NOT_PAUSED)
recoveryPausesHere(false);
}
@@ -12624,6 +12672,11 @@ WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
elog(ERROR, "unexpected WAL source %d", currentSource);
}
+ /* test for recovery pause, if user has requested the pause */
+ if (((volatile XLogCtlData *) XLogCtl)->recoveryPauseState !=
+ RECOVERY_NOT_PAUSED)
+ recoveryPausesHere(false);
+
/*
* This possibly-long loop needs to handle interrupts of startup
* process.
diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c
index d8c5bf6..710fc25 100644
--- a/src/backend/access/transam/xlogfuncs.c
+++ b/src/backend/access/transam/xlogfuncs.c
@@ -517,7 +517,7 @@ pg_walfile_name(PG_FUNCTION_ARGS)
}
/*
- * pg_wal_replay_pause - pause recovery now
+ * pg_wal_replay_pause - Request to pause recovery
*
* Permission checking for this function is managed through the normal
* GRANT system.
@@ -540,6 +540,9 @@ pg_wal_replay_pause(PG_FUNCTION_ARGS)
SetRecoveryPause(true);
+ /* wake up the recovery process so that it can process the pause request */
+ WakeupRecovery();
+
PG_RETURN_VOID();
}
@@ -582,7 +585,46 @@ pg_is_wal_replay_paused(PG_FUNCTION_ARGS)
errmsg("recovery is not in progress"),
errhint("Recovery control functions can only be executed during recovery.")));
- PG_RETURN_BOOL(RecoveryIsPaused());
+ PG_RETURN_BOOL(GetRecoveryPauseState() != RECOVERY_NOT_PAUSED);
+}
+
+/*
+ * pg_get_wal_replay_pause_state - Returns the recovery pause state.
+ *
+ * Returned values:
+ *
+ * 'not paused' - if pause is not requested
+ * 'pause requested' - if pause is requested but recovery is not yet paused
+ * 'paused' - if recovery is paused
+ */
+Datum
+pg_get_wal_replay_pause_state(PG_FUNCTION_ARGS)
+{
+ char *state;
+
+ if (!RecoveryInProgress())
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("recovery is not in progress"),
+ errhint("Recovery control functions can only be executed during recovery.")));
+
+ /* get the recovery pause state */
+ switch(GetRecoveryPauseState())
+ {
+ case RECOVERY_NOT_PAUSED:
+ state = "not paused";
+ break;
+ case RECOVERY_PAUSE_REQUESTED:
+ state = "pause requested";
+ break;
+ case RECOVERY_PAUSED:
+ state = "paused";
+ break;
+ default:
+ elog(ERROR, "invalid recovery pause state");
+ }
+
+ PG_RETURN_TEXT_P(cstring_to_text(state));
}
/*
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index 75ec107..8f0efd5 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -174,6 +174,14 @@ typedef enum RecoveryState
RECOVERY_STATE_DONE /* currently in production */
} RecoveryState;
+/* Recovery pause states */
+typedef enum RecoveryPauseState
+{
+ RECOVERY_NOT_PAUSED, /* pause not requested */
+ RECOVERY_PAUSE_REQUESTED, /* pause requested, but not yet paused */
+ RECOVERY_PAUSED /* recovery is paused */
+} RecoveryPauseState;
+
extern PGDLLIMPORT int wal_level;
/* Is WAL archiving enabled (always or only while server is running normally)? */
@@ -310,7 +318,7 @@ extern void GetXLogReceiptTime(TimestampTz *rtime, bool *fromStream);
extern XLogRecPtr GetXLogReplayRecPtr(TimeLineID *replayTLI);
extern XLogRecPtr GetXLogInsertRecPtr(void);
extern XLogRecPtr GetXLogWriteRecPtr(void);
-extern bool RecoveryIsPaused(void);
+extern RecoveryPauseState GetRecoveryPauseState(void);
extern void SetRecoveryPause(bool recoveryPause);
extern TimestampTz GetLatestXTime(void);
extern TimestampTz GetCurrentChunkReplayStartTime(void);
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 1604412..764a0f5 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -6230,6 +6230,10 @@
proname => 'pg_is_wal_replay_paused', provolatile => 'v',
prorettype => 'bool', proargtypes => '',
prosrc => 'pg_is_wal_replay_paused' },
+{ oid => '1137', descr => 'get wal replay pause state',
+ proname => 'pg_get_wal_replay_pause_state', provolatile => 'v',
+ prorettype => 'text', proargtypes => '',
+ prosrc => 'pg_get_wal_replay_pause_state' },
{ oid => '2621', descr => 'reload configuration files',
proname => 'pg_reload_conf', provolatile => 'v', prorettype => 'bool',
--
1.8.3.1
