Re: Is Recovery actually paused?

At Tue, 9 Feb 2021 09:47:58 +0530, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote in
> On Tue, Feb 9, 2021 at 8:54 AM Yugo NAGATA <nagata(at)sraoss(dot)co(dot)jp> wrote:
> >
> > On Tue, 09 Feb 2021 10:58:04 +0900 (JST)
> > Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com> wrote:
> >
> > > At Mon, 8 Feb 2021 17:05:52 +0530, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote in
> > > > On Mon, Feb 8, 2021 at 2:19 PM Yugo NAGATA <nagata(at)sraoss(dot)co(dot)jp> wrote:
> > > > >
> > > > > On Mon, 08 Feb 2021 17:32:46 +0900 (JST)
> > > > > Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com> wrote:
> > > > >
> > > > > > At Mon, 8 Feb 2021 14:12:35 +0900, Yugo NAGATA <nagata(at)sraoss(dot)co(dot)jp> wrote 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.
> > > > > >
> > > > > > Is there any actual harm if PAUSED returns to REQUESETED, assuming we
> > > > > > immediately change the state to PAUSE always we see REQUESTED in the
> > > > > > waiting loop, despite that we allow change the state from PAUSE to
> > > > > > REQUESTED via NOT_PAUSED between two successive loop condition checks?
> > > > >
> > > > > If a user call pg_wal_replay_pause while recovery is paused, users can
> > > > > observe 'pause requested' during a sleep alghough the time window is short.
> > > > > It seems a bit odd that pg_wal_replay_pause changes the state like this
> > > > > because This state meeans that recovery may not be 'paused'.
> > > >
> > > > Yeah, this appears wrong that after 'paused' we go back to 'pause
> > > > requested'. the logical state transition should always be as below
> > > >
> > > > NOT PAUSED -> PAUSE REQUESTED or PAUSED (maybe we should always go to
> > > > request and then paused but there is nothing wrong with going to
> > > > paused)
> > > > PAUSE REQUESTED -> NOT PAUSE or PAUSED (either cancel the request or get paused)
> > > > PAUSED -> NOT PAUSED (from PAUSED we should not go to the
> > > > PAUSE_REQUESTED without going to NOT PAUSED)
> > >
> > > I didn't asked about the internal logical correctness, but asked about
> > > *actual harm* revealed to users. I don't see any actual harm in the
> > > "wrong" transition because:
> >
> > Actually, the incorrect state transition is not so harmful except that
> > users can observe unnecessary state changes. However, I don't think any
> > actual harm in prohibit the incorrect state transition. So, I think we
> > can do it.
> >
> > > If we are going to introduce that complexity, I'd like to re-propose
> > > to introduce interlocking between the recovery side and the
> > > pause-requestor side instead of introducing the intermediate state,
> > > which is the cause of the complexity.
> > >
> > > The attached PoC patch adds:
> > >
> > > - A solid checkpoint just before calling rm_redo. It doesn't add a
> > > info_lck since the check is done in the existing lock section.
> > >
> > > - Interlocking between the above and SetRecoveryPause without adding a
> > > shared variable.
> > > (This is what I called "synchronous" before.)
> >
> > I think waiting in pg_wal_replay_pasue is a possible option, but this will
> > also introduce other complexity to codes such as possibility of waiting for
> > long or for ever. For example, waiting in SetRecoveryPause as in your POC
> > patch appears to make recovery stuck in RecoveryRequiresIntParameter.

Ah. Yes, startup process does not need to wait. That is a bug of the
patch. No other callers don't cause the self dead lock.

> I agree with this, I think we previously discussed these approaches
> where we can wait in pg_wal_replay_pasue() or
> pg_is_wal_replay_pasued(). In fact, we had an older version where we
> put the wait in pg_is_wal_replay_pasued(). But it appeared that doing

Note that the expected waiting period is while calling rmgr_redo(). If
it is stuck for a long time, that suggests something's going wrong.

> so will add extra complexity as well as instead of waiting in these
> APIs the wait logic can be implemented in the application code which
> is actually using these APIs and IMHO that will give better control to
> the users.

Year, with the PoC pg_wal_replay_pause() can make a short wait as a
side-effect but the tri-state patch also can add a function to wait
for the state suffices.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center