On Sun, Jan 24, 2021 at 12:17 PM Bharath Rupireddy
<bharath.rupireddyforpostg...@gmail.com> wrote:
>
> On Sun, Jan 24, 2021 at 11:29 AM Dilip Kumar <dilipbal...@gmail.com> wrote:
> > > Some comments on the v6 patch:
>
> > > [2] Typo - it's "requested" + * 'paused requested' - if pause is
> > > reqested but recovery is not yet paused
>
> Here I meant the typo "reqested" in "if pause is reqested but recovery
> is not yet paused" statement from v6 patch.
Ok
> > > [3] I think it's "+ * 'pause requested'" instead of "+ * 'paused
> > > requested'"
> >
> > Which code does it refer, can give put the snippet from the patch.
> > However, I have found there were 'paused requested' in two places so I
> > have fixed.
>
> Thanks.
>
> > > [6] As I mentioned upthread, isn't it better to have
> > > "IsRecoveryPaused(void)" than "RecoveryIsPaused(void)"?
> >
> > That is an existing function so I think it's fine to keep the same name.
>
> Personally, I think the function RecoveryIsPaused itself is
> unnecessary with the new function GetRecoveryPauseState introduced in
> your patch. IMHO, we can remove it. If not okay, then we are at it,
> can we at least change the function name to be meaningful
> "IsRecoveryPaused"? Others may have better thoughts than me.
I have removed this function
> > > [7] Can we have the function variable name "recoveryPause" as "state"
> > > or "pauseState? Because that variable context is set by the enum name
> > > RecoveryPauseState and the function name.
> > >
> > > +SetRecoveryPause(RecoveryPauseState recoveryPause)
> > >
> > > Here as well, "recoveryPauseState" to "state"?
> > > +GetRecoveryPauseState(void)
> > > {
> > > - bool recoveryPause;
> > > + RecoveryPauseState recoveryPauseState;
> >
> > I don't think it is required but while changing the patch I will see
> > whether to change or not.
>
> It will be good to change that. I personally don't like structure
> names and variable names to be the same.
Changed to state
> > > [6] Function name RecoveryIsPaused and it's comment "Check whether the
> > > recovery pause is requested." doesn't seem to be matching. Seems like
> > > it returns true even when RECOVERY_PAUSE_REQUESTED or RECOVERY_PAUSED.
> > > Should it return true only when the state is RECOVERY_PAUSE_REQUESTED?
> >
> > Code is doing right, I will change the comments.
> >
> > > Instead of "while (RecoveryIsPaused())", can't we change it to "while
> > > (GetRecoveryPauseState() != RECOVERY_NOT_PAUSED)" and remove the
> > > RecoveryIsPaused()?
> >
> > I think it looks clean with the function
>
> As I said earlier, I see no use of RecoveryIsPaused() with the
> introduction of the new function GetRecoveryPauseState(). Others may
> have better thoughts than me.
>
> > > [7] Can we change the switch-case in pg_is_wal_replay_paused to
> > > something like below?
> > >
> > > Datum
> > > pg_is_wal_replay_paused(PG_FUNCTION_ARGS)
> > > {
> > > + char *state;
> > > + /* get the recovery pause state */
> > > + switch(GetRecoveryPauseState())
> > > + {
> > > + case RECOVERY_NOT_PAUSED:
> > > + state = "not paused";
> > > + case RECOVERY_PAUSE_REQUESTED:
> > > + state = "paused requested";
> > > + case RECOVERY_PAUSED:
> > > + state = "paused";
> > > + default:
> > > + elog(ERROR, "invalid recovery pause state");
> > > + }
> > > +
> > > + PG_RETURN_TEXT_P(cstring_to_text(type));
> >
> > Why do you think it is better to use an extra variable?
>
> I see no wrong in having PG_RETURN_TEXT_P and cstring_to_text 's in
> every case statement. But, just to make sure the code looks cleaner, I
> said that we can have a local state variable and just one
> PG_RETURN_TEXT_P(cstring_to_text(state));. See some existing functions
> brin_page_type, hash_page_type, json_typeof,
> pg_stat_get_backend_activity, pg_stat_get_backend_wait_event_type,
> pg_stat_get_backend_wait_event, get_command_type.
>
I have changed as per other functions for consistency.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
From 5a42c06f866f4e52e136ac46092b3e7f75e9c3ec Mon Sep 17 00:00:00 2001
From: Dilip Kumar <dilipkumar@localhost.localdomain>
Date: Sat, 23 Jan 2021 11:59:58 +0530
Subject: [PATCH v7] pg_is_wal_replay_paused will return the status of recovery
pause
Currently, pg_is_wal_replay_paused, just check 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. With this
patch the API 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 | 13 +++--
src/backend/access/transam/xlog.c | 88 +++++++++++++++++++++++-----------
src/backend/access/transam/xlogfuncs.c | 35 ++++++++++++--
src/include/access/xlog.h | 12 ++++-
src/include/catalog/pg_proc.dat | 2 +-
5 files changed, 109 insertions(+), 41 deletions(-)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index aa99665..8139fdc 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -25221,7 +25221,10 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup());
<returnvalue>boolean</returnvalue>
</para>
<para>
- Returns true if recovery is paused.
+ Returns recovery pause status, which is <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>
@@ -25260,10 +25263,10 @@ 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. 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 470e113..bcef94d 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -725,8 +725,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
@@ -898,6 +898,7 @@ static void validateRecoveryParameters(void);
static void exitArchiveRecovery(TimeLineID endTLI, XLogRecPtr endOfLog);
static bool recoveryStopsBefore(XLogReaderState *record);
static bool recoveryStopsAfter(XLogReaderState *record);
+static void CheckAndSetRecoveryPause(void);
static void recoveryPausesHere(bool endOfRecovery);
static bool recoveryApplyDelay(XLogReaderState *record);
static void SetLatestXTime(TimestampTz xtime);
@@ -6023,7 +6024,20 @@ recoveryStopsAfter(XLogReaderState *record)
}
/*
- * Wait until shared recoveryPause flag is cleared.
+ * If recovery pause is requested then set its state as paused.
+ */
+static void
+CheckAndSetRecoveryPause(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);
+}
+
+/*
+ * Wait until shared recoveryPause 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
@@ -6053,34 +6067,50 @@ recoveryPausesHere(bool endOfRecovery)
(errmsg("recovery has paused"),
errhint("Execute pg_wal_replay_resume() to continue.")));
- while (RecoveryIsPaused())
+ while (GetRecoveryPauseState() != RECOVERY_NOT_PAUSED)
{
+
HandleStartupProcInterrupts();
+
if (CheckForStandbyTrigger())
return;
pgstat_report_wait_start(WAIT_EVENT_RECOVERY_PAUSE);
+
+ /*
+ * While we are in the loop, user might resume and pause again so set
+ * this every time.
+ */
+ CheckAndSetRecoveryPause();
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.
+ */
void
-SetRecoveryPause(bool recoveryPause)
+SetRecoveryPause(RecoveryPauseState state)
{
+ Assert(state >= RECOVERY_NOT_PAUSED && state <= RECOVERY_PAUSED);
+
SpinLockAcquire(&XLogCtl->info_lck);
- XLogCtl->recoveryPause = recoveryPause;
+ XLogCtl->recoveryPauseState = state;
SpinLockRelease(&XLogCtl->info_lck);
}
@@ -6172,6 +6202,11 @@ recoveryApplyDelay(XLogReaderState *record)
WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
msecs,
WAIT_EVENT_RECOVERY_APPLY_DELAY);
+
+ /* test for recovery pause, if user has requested the pause */
+ if (((volatile XLogCtlData *) XLogCtl)->recoveryPauseState ==
+ RECOVERY_PAUSE_REQUESTED)
+ recoveryPausesHere(false);
}
return true;
}
@@ -6274,14 +6309,14 @@ RecoveryRequiresIntParameter(const char *param_name, int currValue, int minValue
currValue,
minValue)));
- SetRecoveryPause(true);
+ SetRecoveryPause(RECOVERY_PAUSE_REQUESTED);
ereport(LOG,
(errmsg("recovery has paused"),
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();
@@ -7192,7 +7227,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 */
@@ -7296,7 +7331,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_PAUSE_REQUESTED)
recoveryPausesHere(false);
/*
@@ -7312,18 +7348,7 @@ StartupXLOG(void)
* If we've been asked to lag the primary, wait on latch until
* enough time has passed.
*/
- if (recoveryApplyDelay(xlogreader))
- {
- /*
- * We test for paused recovery again here. If user sets
- * delayed apply, it may be because they expect to pause
- * recovery in case of problems, so we must test again
- * here otherwise pausing during the delay-wait wouldn't
- * work.
- */
- if (((volatile XLogCtlData *) XLogCtl)->recoveryPause)
- recoveryPausesHere(false);
- }
+ recoveryApplyDelay(xlogreader);
/* Setup error traceback support for ereport() */
errcallback.callback = rm_redo_error_callback;
@@ -7495,7 +7520,7 @@ StartupXLOG(void)
proc_exit(3);
case RECOVERY_TARGET_ACTION_PAUSE:
- SetRecoveryPause(true);
+ SetRecoveryPause(RECOVERY_PAUSE_REQUESTED);
recoveryPausesHere(true);
/* drop into promote */
@@ -12648,6 +12673,13 @@ 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_PAUSE_REQUESTED)
+ recoveryPausesHere(false);
+
+ now = GetCurrentTimestamp();
+
/*
* 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 5e1aab3..ceac8a2 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.
@@ -538,7 +538,10 @@ pg_wal_replay_pause(PG_FUNCTION_ARGS)
errhint("%s cannot be executed after promotion is triggered.",
"pg_wal_replay_pause()")));
- SetRecoveryPause(true);
+ SetRecoveryPause(RECOVERY_PAUSE_REQUESTED);
+
+ /* wake up the recovery process so that it can process the pause request */
+ WakeupRecovery();
PG_RETURN_VOID();
}
@@ -565,24 +568,46 @@ pg_wal_replay_resume(PG_FUNCTION_ARGS)
errhint("%s cannot be executed after promotion is triggered.",
"pg_wal_replay_resume()")));
- SetRecoveryPause(false);
+ SetRecoveryPause(RECOVERY_NOT_PAUSED);
PG_RETURN_VOID();
}
/*
- * pg_is_wal_replay_paused
+ * pg_is_wal_replay_paused - Returns the current state of the recovery pause
+ *
+ * '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_is_wal_replay_paused(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.")));
- PG_RETURN_BOOL(RecoveryIsPaused());
+ /* 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..7533c48 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 = 0, /* pause not requested */
+ RECOVERY_PAUSE_REQUESTED = 1, /* pause requested, but yet paused */
+ RECOVERY_PAUSED = 2 /* recovery is paused */
+} RecoveryPauseState;
+
extern PGDLLIMPORT int wal_level;
/* Is WAL archiving enabled (always or only while server is running normally)? */
@@ -310,8 +318,8 @@ 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 void SetRecoveryPause(bool recoveryPause);
+extern RecoveryPauseState GetRecoveryPauseState(void);
+extern void SetRecoveryPause(RecoveryPauseState state);
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 b5f52d4..6ff673a 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -6220,7 +6220,7 @@
proargtypes => '', prosrc => 'pg_wal_replay_resume' },
{ oid => '3073', descr => 'true if wal replay is paused',
proname => 'pg_is_wal_replay_paused', provolatile => 'v',
- prorettype => 'bool', proargtypes => '',
+ prorettype => 'text', proargtypes => '',
prosrc => 'pg_is_wal_replay_paused' },
{ oid => '2621', descr => 'reload configuration files',
--
1.8.3.1
