Re: Is Recovery actually paused?

On Thu, Feb 11, 2021 at 6:07 AM Dilip Kumar <dilipbalaut(at)gmail(dot)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:

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

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

- 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. 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).

- 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().

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

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

--
Robert Haas
EDB: http://www.enterprisedb.com