Re: Is Recovery actually paused?

On Fri, Feb 12, 2021 at 3:26 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Thu, Feb 11, 2021 at 6:07 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > > Thanks for the patch. I tested the new function and it works as
> > > expected. I have no further comments on the v13 patch.
> >
> > Thanks for the review and testing.
>
> I don't see a whole lot wrong with this patch, but I think there are
> some things that could make it a little clearer:

Thanks for the review

> - I suggest renaming CheckAndSetRecoveryPause() to ConfirmRecoveryPaused().

Yeah that make more sense so changed.

> - I suggest moving the definition of that function to just after
> SetRecoveryPause().

Done

> - I suggest changing the argument to SetRecoveryPause() back to bool.
> In the one place where you call SetRecoveryPause(RECOVERY_PAUSED),
> just call SetRecoveryPause(true) and ConfirmRecoveryPaused() back to
> back.

Yeah done that way,  I think only in once place we were doing
SetRecoveryPause(RECOVERY_PAUSED), but after putting more thought I
think that was not required because right after setting that we are
having the while loop under that we have to call
ConfirmRecoveryPaused.  So I have changed that also as
SetRecoveryPause(true) without immediate call of
ConfirmRecoveryPaused.

This in turn means that the "if" statement in
> SetRecoveryPaused() can be rewritten as if (!recoveryPaused)
> XLogCtl->recoveryPauseState = RECOVERY_NOT_PAUSED else if
> (XLogCtl->recoveryPauseState == RECOVERY_NOT_PAUSED)
> XLogCtl->recoveryPauseState = RECOVERY_PAUSE_REQUESTED(). This is
> slightly less efficient, but I don't think it matters, and I think it
> will be a lot more clear what's the job of SetRecoveryPause (say
> whether we're trying to pause or not) and what's the job of
> ConfirmRecoveryPaused (say whether we've succeeded in pausing).

Done

> - Since the numeric values of RecoveryPauseState don't matter and the
> values are never visible to anything outside the server nor stored on
> disk, I would be inclined to (a) not specify particular values in
> xlog.h and (b) remove the test-and-elog in SetRecoveryPause().

Done

> - In the places where you say:
>
> -                if (((volatile XLogCtlData *) XLogCtl)->recoveryPause)
> +                if (((volatile XLogCtlData *) XLogCtl)->recoveryPauseState ==
> +                    RECOVERY_PAUSE_REQUESTED)
>
> ...I would suggest instead testing for != RECOVERY_NOT_PAUSED. Perhaps
> we don't think RECOVERY_PAUSED can happen here. But if somehow it did,
> calling recoveryPausesHere() would be right.

Done

> There might be some more to say here, but those are things I notice on
> a first read-through.

Okay.

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