From: | Dilip Kumar <dilipbalaut(at)gmail(dot)com> |
---|---|
To: | Robert Haas <robertmhaas(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-23 06:33:32 |
Message-ID: | CAFiTN-s5CjQQjd_W0X95hvD1HbD6Jm3EJjhHmvdASkyk9NS2Pw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Fri, Feb 12, 2021 at 3:26 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> 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:
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
Attachment | Content-Type | Size |
---|---|---|
v14-0001-Provide-a-new-interface-to-get-the-recovery-paus.patch | text/x-patch | 13.0 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Thomas Munro | 2021-02-23 07:23:19 | Re: pgsql: pg_collation_actual_version() -> pg_collation_current_version(). |
Previous Message | Amit Kapila | 2021-02-23 06:30:38 | Re: locking [user] catalog tables vs 2pc vs logical rep |