Re: Is Recovery actually paused?

On Thu, Feb 4, 2021 at 6:16 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Thu, Feb 4, 2021 at 4:58 PM Bharath Rupireddy
> <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> >
> > On Thu, Feb 4, 2021 at 10:28 AM Dilip Kumar <dilipbalaut(at)gmail(dot)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.
> > */
>
> How can we do that this is not a 1 byte flag this is enum so I don't
> think we can read any atomic state without a spin lock here.

I have fixed the other comments and the updated patch is attached.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com