Re: Is Recovery actually paused?

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. 

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().

Regrads,
Yugo Nagata

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