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

On Fri, Jan 29, 2021 at 4:33 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Fri, Jan 29, 2021 at 3:25 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote:
> >
> > On Thu, 28 Jan 2021 09:55:42 +0530
> > Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > > On Wed, Jan 27, 2021 at 2:28 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > > >
> > > > On Wed, Jan 27, 2021 at 2:06 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote:
> > > > >
> > > > > On Wed, 27 Jan 2021 13:29:23 +0530
> > > > > Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > > > >
> > > > > > On Wed, Jan 27, 2021 at 12:50 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > > > > >
> > > > > > > On Tue, Jan 26, 2021 at 2:00 AM Robert Haas <robertmhaas@gmail.com> wrote:
> > > > > > > >
> > > > > > > > On Sat, Jan 23, 2021 at 6:10 AM Bharath Rupireddy
> > > > > > > > <bharath.rupireddyforpostgres@gmail.com> wrote:
> > > > > > > > > +1 to just show the recovery pause state in the output of
> > > > > > > > > pg_is_wal_replay_paused. But, should the function name
> > > > > > > > > "pg_is_wal_replay_paused" be something like
> > > > > > > > > "pg_get_wal_replay_pause_state" or some other? To me, when "is" exists
> > > > > > > > > in a function, I expect a boolean output. Others may have better
> > > > > > > > > thoughts.
> > > > > > > >
> > > > > > > > Maybe we should leave the existing function pg_is_wal_replay_paused()
> > > > > > > > alone and add a new one with the name you suggest that returns text.
> > > > > > > > That would create less burden for tool authors.
> > > > > > >
> > > > > > > +1
> > > > > > >
> > > > > >
> > > > > > Yeah, we can do that, I will send an updated patch soon.
> > > > >
> > > > > This means pg_is_wal_replay_paused is left without any change and this
> > > > > returns whether pause is requested or not? If so, it seems good to modify
> > > > > the documentation of this function in order to note that this could not
> > > > > return the actual pause state.
> > > >
> > > > Yes, we can say that it will return true if the replay pause is
> > > > requested.  I am changing that in my new patch.
> > >
> > > I have modified the patch, changes
> > >
> > > - I have added a new interface pg_get_wal_replay_pause_state to get
> > > the pause request state
> > > - Now, we are not waiting for the recovery to actually get paused so I
> > > think it doesn't make sense to put a lot of checkpoints to check the
> > > pause requested so I have removed that check from the
> > > recoveryApplyDelay but I think it better we still keep that check in
> > > the WaitForWalToBecomeAvailable because it can wait forever before the
> > > next wal get available.
> >
> > I think basically the check in WaitForWalToBecomeAvailable is independent
> > of the feature of pg_get_wal_replay_pause_state, that is, reporting the
> > actual pause state.  This function could just return 'pause requested'
> > if a pause is requested during waiting for WAL.
> >
> > However, I agree the change to allow recovery to transit the state to
> > 'paused' during WAL waiting because 'paused' has more useful information
> > for users than 'pause requested'.  Returning 'paused' lets users know
> > clearly that no more WAL are applied until recovery is resumed.  On the
> > other hand, when 'pause requested' is returned, user can't say whether
> > the next WAL wiill be applied or not from this information.
> >
> > For the same reason, I think it is also useful to call recoveryPausesHere
> > in recoveryApplyDelay.
>
> IMHO the WaitForWalToBecomeAvailable can wait until the next wal get
> available so it can not be controlled by user so it is good to put a
> check for the recovery pause,  however recoveryApplyDelay wait for the
> apply delay which is configured by user and it is predictable value by
> the user.  I don't have much objection to putting that check in the
> recoveryApplyDelay as well but I feel it is not necessary.  Any other
> thoughts on this?
>
> > In addition, in RecoveryRequiresIntParameter, recovery should get paused
> > if a parameter value has a problem.  However, pg_get_wal_replay_pause_state
> > will return 'pause requested' in this case. So, I think, we should pass
> > RECOVERY_PAUSED to SetRecoveryPause() instead of RECOVERY_PAUSE_REQUESTED,
> > or call CheckAndSetRecoveryPause() in the loop like recoveryPausesHere().
>
> Yeah, absolutely right, it must pass RECOVERY_PAUSED.  I will change
> this, thanks for noticing this.

I have changed this in the new patch.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com