On Thu, Feb 11, 2021 at 6:07 AM Dilip Kumar <dilipbal...@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: - 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