Re: Is Recovery actually paused?

On Thu, Feb 4, 2021 at 10:28 AM Dilip Kumar <dilipbalaut@gmail.com> 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:

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.
                 */

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com