Re: Is Recovery actually paused? - Mailing list pgsql-hackers

On Mon, 8 Feb 2021 09:35:00 +0530
Dilip Kumar <dilipbalaut@gmail.com> wrote:

> On Mon, Feb 8, 2021 at 8:18 AM Yugo NAGATA <nagata@sraoss.co.jp> wrote:
> >
> > On Mon, 8 Feb 2021 07:51:22 +0530
> > Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > > On Mon, 8 Feb 2021 at 6:38 AM, Yugo NAGATA <nagata@sraoss.co.jp> wrote:
> > >
> > > > Hi,
> > > >
> > > > On Sun, 7 Feb 2021 19:27:02 +0530
> > > > Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > > >
> > > > > On Sun, Feb 7, 2021 at 6:44 PM Bharath Rupireddy
> > > > > <bharath.rupireddyforpostgres@gmail.com> wrote:
> > > > > >
> > > > > > On Fri, Feb 5, 2021 at 10:14 AM Bharath Rupireddy
> > > > > > <bharath.rupireddyforpostgres@gmail.com> wrote:
> > > > > > > > We can not do that, basically, under one lock we need to check the
> > > > > > > > state and set it to pause.  Because by the time you release the
> > > > lock
> > > > > > > > someone might set it to RECOVERY_NOT_PAUSED then you don't want to
> > > > set
> > > > > > > > it to RECOVERY_PAUSED.
> > > > > > >
> > > > > > > Got it. Thanks.
> > > > > >
> > > > > > Hi Dilip, I have one more question:
> > > > > >
> > > > > > +        /* test for recovery pause, if user has requested the pause */
> > > > > > +        if (((volatile XLogCtlData *) XLogCtl)->recoveryPauseState ==
> > > > > > +            RECOVERY_PAUSE_REQUESTED)
> > > > > > +            recoveryPausesHere(false);
> > > > > > +
> > > > > > +        now = GetCurrentTimestamp();
> > > > > > +
> > > > > >
> > > > > > Do we need  now = GetCurrentTimestamp(); here? Because, I see that
> > > > > > whenever the variable now is used within the for loop in
> > > > > > WaitForWALToBecomeAvailable, it's re-calculated anyways. It's being
> > > > > > used within case XLOG_FROM_STREAM:
> > > > > >
> > > > > > Am I missing something?
> > > > >
> > > > > Yeah, I don't see any reason for doing this, maybe it got copy pasted
> > > > > by mistake.  Thanks for observing this.
> > > >
> > > > I also have a question:
> > > >
> > > > @@ -6270,14 +6291,14 @@ RecoveryRequiresIntParameter(const char
> > > > *param_name, int currValue, int minValue
> > > >                                                            currValue,
> > > >                                                            minValue)));
> > > >
> > > > -                       SetRecoveryPause(true);
> > > > +                       SetRecoveryPause(RECOVERY_PAUSED);
> > > >
> > > >                         ereport(LOG,
> > > >                                         (errmsg("recovery has paused"),
> > > >                                          errdetail("If recovery is
> > > > unpaused, the server will shut down."),
> > > >                                          errhint("You can then restart the
> > > > server after making the necessary configuration changes.")));
> > > >
> > > > -                       while (RecoveryIsPaused())
> > > > +                       while (GetRecoveryPauseState() !=
> > > > RECOVERY_NOT_PAUSED)
> > > >                         {
> > > >                                 HandleStartupProcInterrupts();
> > > >
> > > >
> > > >
> > > > If a user call pg_wal_replay_pause while waiting in
> > > > RecoveryRequiresIntParameter,
> > > > the state become 'pause requested' and this never returns to 'paused'.
> > > > Should we check recoveryPauseState in this loop as in
> > >
> > >
> > > I think the right fix should be that the state should never go from
> > > ‘paused’ to ‘pause requested’  so I think pg_wal_replay_pause should take
> > > care of that.
> >
> > It makes sense to take care of this in pg_wal_replay_pause, but I wonder
> > it can not handle the case that a user resume and pause again while a sleep.
> 
> Right, we will have to check and set in the loop.  But we should not
> allow the state to go from paused to pause requested irrespective of
> this.

I agree with you.

-- 
Yugo NAGATA <nagata@sraoss.co.jp>