On Thu, Feb 4, 2021 at 4:58 PM Bharath Rupireddy
<[email protected]> wrote:
>
> On Thu, Feb 4, 2021 at 10:28 AM Dilip Kumar <[email protected]> wrote:
> > Please find an updated patch which addresses these comments.
>
> Thanks for the patch. I tested the new function pg_get_wal_replay_pause_state:
>
> postgres=# select pg_get_wal_replay_pause_state();
> pg_get_wal_replay_pause_state
> -------------------------------
> not paused
> postgres=# select pg_wal_replay_pause();
> pg_wal_replay_pause
> ---------------------
>
> (1 row)
>
> I can also see the "pause requested" state after I put a gdb
> breakpoint in WaitForWALToBecomeAvailable in the standby startup
> process .
>
> postgres=# select pg_get_wal_replay_pause_state();
> pg_get_wal_replay_pause_state
> -------------------------------
> pause requested
> (1 row)
>
> postgres=# select pg_get_wal_replay_pause_state();
> pg_get_wal_replay_pause_state
> -------------------------------
> paused
> (1 row)
>
> Mostly, the v10 patch looks good to me, except below minor comments:
Thanks for the testing.
> 1) A typo in commit message - "just check" --> "just checks"
>
> 2) How about
> + Returns recovery pause state. The return values are <literal>not
> paused
> instead of
> + Returns recovery pause state, the return values are <literal>not
> paused
>
> 3) I think it is 'get wal replay pause state', instead of { oid =>
> '1137', descr => 'get wal replay is pause state',
>
> 4) can we just do this
> /*
> * 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.
> */
> if (((volatile XLogCtlData *) XLogCtl)->recoveryPauseState ==
> RECOVERY_PAUSE_REQUESTED)
> SetRecoveryPause(RECOVERY_PAUSED);
> instead of
> /*
> * 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.
> */
> SpinLockAcquire(&XLogCtl->info_lck);
> if (XLogCtl->recoveryPauseState == RECOVERY_PAUSE_REQUESTED)
> XLogCtl->recoveryPauseState = RECOVERY_PAUSED;
> SpinLockRelease(&XLogCtl->info_lck);
>
> I think it's okay, since we take a spinlock anyways in
> GetRecoveryPauseState(). See the below comment and also a relevant
> commit 6ba4ecbf477e0b25dd7bde1b0c4e07fc2da19348 on why it's not
> necessary taking spinlock always:
> /*
> * Pause WAL replay, if requested by a hot-standby session via
> * SetRecoveryPause().
> *
> * Note that we intentionally don't take the info_lck spinlock
> * here. We might therefore read a slightly stale value of
> * the recoveryPause flag, but it can't be very stale (no
> * worse than the last spinlock we did acquire). Since a
> * pause request is a pretty asynchronous thing anyway,
> * possibly responding to it one WAL record later than we
> * otherwise would is a minor issue, so it doesn't seem worth
> * adding another spinlock cycle to prevent that.
> */
>
I will work on these comments and send the updated patch soon.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
