From: | Robert Haas <robertmhaas(at)gmail(dot)com> |
---|---|
To: | Dilip Kumar <dilipbalaut(at)gmail(dot)com> |
Cc: | Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>, Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>, Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Simon Riggs <simon(at)2ndquadrant(dot)com> |
Subject: | Re: Is Recovery actually paused? |
Date: | 2021-02-11 21:56:06 |
Message-ID: | CA+TgmoYKGJYyR4iPVuGLCi4usXN1phe167W+YOFUUDGZb+BasQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Ranier Vilela | 2021-02-11 21:56:13 | Re: Operands don't affect result (CONSTANT_EXPRESSION_RESULT) (src/backend/utils/adt/jsonfuncs.c) |
Previous Message | Joel Jacobson | 2021-02-11 21:24:44 | Re: [HACKERS] GSoC 2017: Foreign Key Arrays |