On Sun, Jan 17, 2021 at 1:48 PM Dilip Kumar <dilipbal...@gmail.com> wrote:
>
> On Sat, Jan 16, 2021 at 8:59 AM Masahiko Sawada <sawada.m...@gmail.com> wrote:
> >
> > On Wed, Jan 13, 2021 at 9:20 PM Dilip Kumar <dilipbal...@gmail.com> wrote:
> > >
> > > On Wed, Jan 13, 2021 at 3:35 PM Dilip Kumar <dilipbal...@gmail.com> wrote:
> > > >
> > > > On Wed, Jan 13, 2021 at 3:27 PM Yugo NAGATA <nag...@sraoss.co.jp> wrote:
> > > > >
> > > > > On Thu, 10 Dec 2020 11:25:23 +0530
> > > > > Dilip Kumar <dilipbal...@gmail.com> wrote:
> > > > >
> > > > > > > > However, I wonder users don't expect pg_is_wal_replay_paused to
> > > > > > > > wait.
> > > > > > > > Especially, if max_standby_streaming_delay is -1, this will be
> > > > > > > > blocked forever,
> > > > > > > > although this setting may not be usual. In addition, some users
> > > > > > > > may set
> > > > > > > > recovery_min_apply_delay for a large. If such users call
> > > > > > > > pg_is_wal_replay_paused,
> > > > > > > > it could wait for a long time.
> > > > > > > >
> > > > > > > > At least, I think we need some descriptions on document to
> > > > > > > > explain
> > > > > > > > pg_is_wal_replay_paused could wait while a time.
> > > > > > >
> > > > > > > Ok
> > > > > >
> > > > > > Fixed this, added some comments in .sgml as well as in function
> > > > > > header
> > > > >
> > > > > Thank you for fixing this.
> > > > >
> > > > > Also, is it better to fix the description of pg_wal_replay_pause from
> > > > > "Pauses recovery." to "Request to pause recovery." in according with
> > > > > pg_is_wal_replay_paused?
> > > >
> > > > Okay
> > > >
> > > > >
> > > > > > > > Also, how about adding a new boolean argument to
> > > > > > > > pg_is_wal_replay_paused to
> > > > > > > > control whether this waits for recovery to get paused or not?
> > > > > > > > By setting its
> > > > > > > > default value to true or false, users can use the old format
> > > > > > > > for calling this
> > > > > > > > and the backward compatibility can be maintained.
> > > > > > >
> > > > > > > So basically, if the wait_recovery_pause flag is false then we
> > > > > > > will
> > > > > > > immediately return true if the pause is requested? I agree that
> > > > > > > it is
> > > > > > > good to have an API to know whether the recovery pause is
> > > > > > > requested or
> > > > > > > not but I am not sure is it good idea to make this API serve both
> > > > > > > the
> > > > > > > purpose? Anyone else have any thoughts on this?
> > > > > > >
> > > > >
> > > > > I think the current pg_is_wal_replay_paused() already has another
> > > > > purpose;
> > > > > this waits recovery to actually get paused. If we want to limit this
> > > > > API's
> > > > > purpose only to return the pause state, it seems better to fix this
> > > > > to return
> > > > > the actual state at the cost of lacking the backward compatibility.
> > > > > If we want
> > > > > to know whether pause is requested, we may add a new API like
> > > > > pg_is_wal_replay_paluse_requeseted(). Also, if we want to wait
> > > > > recovery to actually
> > > > > get paused, we may add an option to pg_wal_replay_pause() for this
> > > > > purpose.
> > > > >
> > > > > However, this might be a bikeshedding. If anyone don't care that
> > > > > pg_is_wal_replay_paused() can make user wait for a long time, I don't
> > > > > care either.
> > > >
> > > > I don't think that it will be blocked ever, because
> > > > pg_wal_replay_pause is sending the WakeupRecovery() which means the
> > > > recovery process will not be stuck on waiting for the WAL.
> > > >
> > > > > > > > As another comment, while pg_is_wal_replay_paused is blocking,
> > > > > > > > I can not cancel
> > > > > > > > the query. I think CHECK_FOR_INTERRUPTS() is necessary in the
> > > > > > > > waiting loop.
> > > > >
> > > > > How about this fix? I think users may want to cancel
> > > > > pg_is_wal_replay_paused() during
> > > > > this is blocking.
> > > >
> > > > Yeah, we can do this. I will send the updated patch after putting
> > > > some more thought into these comments. Thanks again for the feedback.
> > > >
> > >
> > > Please find the updated patch.
> >
> > I've looked at the patch. Here are review comments:
> >
> > + /* Recovery pause state */
> > + RecoveryPauseState recoveryPause;
> >
> > Now that the value can have tri-state, how about renaming it to
> > recoveryPauseState?
>
> This makes sense to me.
>
> > ---
> > bool
> > RecoveryIsPaused(void)
> > +{
> > + bool recoveryPause;
> > +
> > + SpinLockAcquire(&XLogCtl->info_lck);
> > + recoveryPause = (XLogCtl->recoveryPause == RECOVERY_PAUSED) ?
> > true : false;
> > + SpinLockRelease(&XLogCtl->info_lck);
> > +
> > + return recoveryPause;
> > +}
> > +
> > +bool
> > +RecoveryPauseRequested(void)
> > {
> > bool recoveryPause;
> >
> > SpinLockAcquire(&XLogCtl->info_lck);
> > - recoveryPause = XLogCtl->recoveryPause;
> > + recoveryPause = (XLogCtl->recoveryPause !=
> > RECOVERY_IN_PROGRESS) ? true : false;
> > SpinLockRelease(&XLogCtl->info_lck);
> >
> > return recoveryPause;
> > }
> >
> > We can write like recoveryPause = (XLogCtl->recoveryPause ==
> > RECOVERY_PAUSED);
>
> In RecoveryPauseRequested, we just want to know whether the pause is
> requested or not, even if the pause requested and not yet pause then
> also we want to return true. So how
> recoveryPause = (XLogCtl->recoveryPause == RECOVERY_PAUSED) will work?
>
> > Also, since these functions do the almost same thing, I think we can
> > have a common function to get XLogCtl->recoveryPause, say
> > GetRecoveryPauseState() or GetRecoveryPause(), and both
> > RecoveryIsPaused() and RecoveryPauseRequested() use the returned
> > value. What do you think?
>
> Yeah we can do that.
>
> > ---
> > +static void
> > +CheckAndSetRecoveryPause(void)
> >
> > Maybe we need to declare the prototype of this function like other
> > functions in xlog.c.
>
> Okay
>
> > ---
> > + /*
> > + * If recovery is not in progress anymore then report an error this
> > + * could happen if the standby is promoted while we were waiting for
> > + * recovery to get paused.
> > + */
> > + 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.")));
> >
> > I think we can improve the error message so that we can tell users the
> > standby has been promoted during the wait. For example,
> >
> > errmsg("the standby was promoted during waiting for
> > recovery to be paused")));
> >
> > ---
> > + /* test for recovery pause if user has requested the pause */
> > + if (((volatile XLogCtlData *) XLogCtl)->recoveryPause)
> > + recoveryPausesHere(false);
> > +
> > + now = GetCurrentTimestamp();
> > +
> >
> > Hmm, if the recovery pauses here, the wal receiver isn't launched even
> > when wal_retrieve_retry_interval has passed, which seems not good. I
> > think we want the recovery to be paused but want the wal receiver to
> > continue receiving WAL.
> >
> > And why do we need to set 'now' here?
> >
> > ---
> > /*
> > * Wait until shared recoveryPause flag is cleared.
> > *
> > * endOfRecovery is true if the recovery target is reached and
> > * the paused state starts at the end of recovery because of
> > * recovery_target_action=pause, and false otherwise.
> > *
> > * XXX Could also be done with shared latch, avoiding the pg_usleep loop.
> > * Probably not worth the trouble though. This state shouldn't be one that
> > * anyone cares about server power consumption in.
> > */
> > static void
> > recoveryPausesHere(bool endOfRecovery)
> >
> > We can improve the first sentence in the above function comment to
> > "Wait until shared recoveryPause is set to RECOVERY_IN_PROGRESS" or
> > something.
> >
> > ---
> > - PG_RETURN_BOOL(RecoveryIsPaused());
> > + if (!RecoveryPauseRequested())
> > + PG_RETURN_BOOL(false);
> > +
> > + /* loop until the recovery is actually paused */
> > + while(!RecoveryIsPaused())
> > + {
> > + pg_usleep(10000L); /* wait for 10 msec */
> > +
> > + /* meanwhile if recovery is resume requested then return false */
> > + if (!RecoveryPauseRequested())
> > + PG_RETURN_BOOL(false);
> > +
> > + CHECK_FOR_INTERRUPTS();
> > +
> > + /*
> > + * If recovery is not in progress anymore then report an error this
> > + * could happen if the standby is promoted while we were waiting for
> > + * recovery to get paused.
> > + */
> > + 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(true);
> >
> > We have the same !RecoveryPauseRequested() check twice, how about the
> > following arrangement?
> >
> > for (;;)
> > {
> > if (!RecoveryPauseRequested())
> > PG_RETURN_BOOL(false);
> >
> > if (RecoveryIsPaused())
> > break;
> >
> > pg_usleep(10000L);
> >
> > CHECK_FOR_INTERRUPTS();
> >
> > if (!RecoveryInProgress())
> > ereport(...);
> > }
> >
> > PG_RETURN_BOOL(true);
> >
> > Regards,
> >
>
> Okay, we can do that. I will make these changes in the next patch.
>
I have fixed the above agreed comments. Please have a look.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
From a99f31114648ae169aad6ef3665890ed27e65ef0 Mon Sep 17 00:00:00 2001
From: Dilip Kumar <dilipkumar@localhost.localdomain>
Date: Thu, 10 Dec 2020 11:22:08 +0530
Subject: [PATCH v4] pg_is_wal_replay_paused will wait for recovery to pause
Currently, pg_is_wal_replay_paused, just check whether the recovery
pause is requested or not but it doesn't actually wait for recovery
to get paused. With this patch if recovery pause is not requested
the api will return false otherwise it will wait for recovery to get
paused.
---
doc/src/sgml/func.sgml | 13 ++++---
src/backend/access/transam/xlog.c | 66 ++++++++++++++++++++++++++--------
src/backend/access/transam/xlogfuncs.c | 42 +++++++++++++++++++---
src/include/access/xlog.h | 11 +++++-
4 files changed, 108 insertions(+), 24 deletions(-)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index fd0370a..c51a1d8 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -25180,7 +25180,10 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup());
<returnvalue>boolean</returnvalue>
</para>
<para>
- Returns true if recovery is paused.
+ If replay pause is requested using <function>pg_wal_replay_pause</function>
+ then this will wait for recovery to actually get paused and once
+ recovery is paused it will return true. Return false if replay pause
+ is not requested.
</para></entry>
</row>
@@ -25219,10 +25222,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 199d911..fc72e5a 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,7 +898,9 @@ 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 RecoveryPauseState GetRecoveryPauseState(void);
static bool recoveryApplyDelay(XLogReaderState *record);
static void SetLatestXTime(TimestampTz xtime);
static void SetCurrentChunkStartTime(TimestampTz xtime);
@@ -6022,8 +6024,18 @@ recoveryStopsAfter(XLogReaderState *record)
return false;
}
+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 flag is cleared.
+ * Wait until shared recoveryPause is set to RECOVERY_IN_PROGRESS.
*
* endOfRecovery is true if the recovery target is reached and
* the paused state starts at the end of recovery because of
@@ -6053,34 +6065,54 @@ recoveryPausesHere(bool endOfRecovery)
(errmsg("recovery has paused"),
errhint("Execute pg_wal_replay_resume() to continue.")));
- while (RecoveryIsPaused())
+ while (RecoveryPauseRequested())
{
+
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 everytime.
+ */
+ CheckAndSetRecoveryPause();
pg_usleep(1000000L); /* 1000 ms */
pgstat_report_wait_end();
}
}
-bool
-RecoveryIsPaused(void)
+static RecoveryPauseState
+GetRecoveryPauseState(void)
{
- bool recoveryPause;
+ RecoveryPauseState recoveryPauseState;
SpinLockAcquire(&XLogCtl->info_lck);
- recoveryPause = XLogCtl->recoveryPause;
+ recoveryPauseState = XLogCtl->recoveryPauseState;
SpinLockRelease(&XLogCtl->info_lck);
- return recoveryPause;
+ return recoveryPauseState;
+}
+
+bool
+RecoveryIsPaused(void)
+{
+ return (GetRecoveryPauseState() == RECOVERY_PAUSED) ? true : false;
+}
+
+bool
+RecoveryPauseRequested(void)
+{
+ return (GetRecoveryPauseState() != RECOVERY_IN_PROGRESS) ? true : false;
}
void
-SetRecoveryPause(bool recoveryPause)
+SetRecoveryPause(RecoveryPauseState recoveryPause)
{
SpinLockAcquire(&XLogCtl->info_lck);
- XLogCtl->recoveryPause = recoveryPause;
+ XLogCtl->recoveryPauseState = recoveryPause;
SpinLockRelease(&XLogCtl->info_lck);
}
@@ -7143,7 +7175,7 @@ StartupXLOG(void)
XLogCtl->lastReplayedTLI = XLogCtl->replayEndTLI;
XLogCtl->recoveryLastXTime = 0;
XLogCtl->currentChunkStartTime = 0;
- XLogCtl->recoveryPause = false;
+ XLogCtl->recoveryPauseState = RECOVERY_IN_PROGRESS;
SpinLockRelease(&XLogCtl->info_lck);
/* Also ensure XLogReceiptTime has a sane value */
@@ -7272,7 +7304,7 @@ StartupXLOG(void)
* here otherwise pausing during the delay-wait wouldn't
* work.
*/
- if (((volatile XLogCtlData *) XLogCtl)->recoveryPause)
+ if (((volatile XLogCtlData *) XLogCtl)->recoveryPauseState)
recoveryPausesHere(false);
}
@@ -7446,7 +7478,7 @@ StartupXLOG(void)
proc_exit(3);
case RECOVERY_TARGET_ACTION_PAUSE:
- SetRecoveryPause(true);
+ SetRecoveryPause(RECOVERY_PAUSE_REQUESTED);
recoveryPausesHere(true);
/* drop into promote */
@@ -12599,6 +12631,12 @@ 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)
+ 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..5847bd3 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,13 +568,17 @@ pg_wal_replay_resume(PG_FUNCTION_ARGS)
errhint("%s cannot be executed after promotion is triggered.",
"pg_wal_replay_resume()")));
- SetRecoveryPause(false);
+ SetRecoveryPause(RECOVERY_IN_PROGRESS);
PG_RETURN_VOID();
}
/*
* pg_is_wal_replay_paused
+ *
+ * If replay pause is requested using pg_wal_replay_pause then this will wait
+ * for recovery to actually get paused and once recovery is paused it will
+ * return true. Return false if replay pause is not requested.
*/
Datum
pg_is_wal_replay_paused(PG_FUNCTION_ARGS)
@@ -582,7 +589,34 @@ 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());
+ /* loop until the recovery is actually paused */
+ for (;;)
+ {
+ /* If recovery pause is not requested then return false */
+ if (!RecoveryPauseRequested())
+ PG_RETURN_BOOL(false);
+
+ if (RecoveryIsPaused())
+ PG_RETURN_BOOL(true);
+
+ /* wait for 10 msec */
+ pg_usleep(10000L);
+
+ CHECK_FOR_INTERRUPTS();
+
+ /*
+ * If recovery is not in progress anymore then report an error this
+ * could happen if the standby is promoted while we were waiting for
+ * recovery to get paused.
+ */
+ if (!RecoveryInProgress())
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("recovery is not in progress"),
+ errhint("The standby was promoted while waiting for recovery to be paused.")));
+ }
+
+ PG_RETURN_BOOL(true);
}
/*
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index 75ec107..d00df1b 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_IN_PROGRESS = 0,
+ RECOVERY_PAUSE_REQUESTED,
+ RECOVERY_PAUSED,
+} RecoveryPauseState;
+
extern PGDLLIMPORT int wal_level;
/* Is WAL archiving enabled (always or only while server is running normally)? */
@@ -311,7 +319,8 @@ extern XLogRecPtr GetXLogReplayRecPtr(TimeLineID *replayTLI);
extern XLogRecPtr GetXLogInsertRecPtr(void);
extern XLogRecPtr GetXLogWriteRecPtr(void);
extern bool RecoveryIsPaused(void);
-extern void SetRecoveryPause(bool recoveryPause);
+extern bool RecoveryPauseRequested(void);
+extern void SetRecoveryPause(RecoveryPauseState recoveryPause);
extern TimestampTz GetLatestXTime(void);
extern TimestampTz GetCurrentChunkReplayStartTime(void);
--
1.8.3.1
