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

On Tue, 19 Jan 2021 21:32:31 +0530
Dilip Kumar <dilipbalaut@gmail.com> wrote:

> On Tue, Jan 19, 2021 at 8:34 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Tue, 19 Jan 2021 at 8:12 AM, Yugo NAGATA <nagata@sraoss.co.jp> wrote:
> >>
> >> On Sun, 17 Jan 2021 11:33:52 +0530
> >> Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >>
> >> > On Thu, Jan 14, 2021 at 6:49 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote:
> >> > >
> >> > > On Wed, 13 Jan 2021 17:49:43 +0530
> >> > > Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >> > >
> >> > > > On Wed, Jan 13, 2021 at 3:35 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >> > > > >
> >> > > > > On Wed, Jan 13, 2021 at 3:27 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote:
> >> > > > > >
> >> > > > > > On Thu, 10 Dec 2020 11:25:23 +0530
> >> > > > > > Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >> > > > > >
> >> > > > > > > > > However, I wonder users don't expect pg_is_wal_replay_paused to wait.
> >> > > > > > > > > Especially, if max_standby_streaming_delay is -1, this will be blocked forever,
> >> > > > > > > > > although this setting may not be usual. In addition, some users may set
> >> > > > > > > > > recovery_min_apply_delay for a large.  If such users call pg_is_wal_replay_paused,
> >> > > > > > > > > it could wait for a long time.
> >> > > > > > > > >
> >> > > > > > > > > At least, I think we need some descriptions on document to explain
> >> > > > > > > > > pg_is_wal_replay_paused could wait while a time.
> >> > > > > > > >
> >> > > > > > > > Ok
> >> > > > > > >
> >> > > > > > > Fixed this, added some comments in .sgml as well as in function header
> >> > > > > >
> >> > > > > > Thank you for fixing this.
> >> > > > > >
> >> > > > > > Also, is it better to fix the description of pg_wal_replay_pause from
> >> > > > > > "Pauses recovery." to "Request to pause recovery." in according with
> >> > > > > > pg_is_wal_replay_paused?
> >> > > > >
> >> > > > > Okay
> >> > > > >
> >> > > > > >
> >> > > > > > > > > Also, how about adding a new boolean argument to pg_is_wal_replay_paused to
> >> > > > > > > > > control whether this waits for recovery to get paused or not? By setting its
> >> > > > > > > > > default value to true or false, users can use the old format for calling this
> >> > > > > > > > > and the backward compatibility can be maintained.
> >> > > > > > > >
> >> > > > > > > > So basically, if the wait_recovery_pause flag is false then we will
> >> > > > > > > > immediately return true if the pause is requested?  I agree that it is
> >> > > > > > > > good to have an API to know whether the recovery pause is requested or
> >> > > > > > > > not but I am not sure is it good idea to make this API serve both the
> >> > > > > > > > purpose?  Anyone else have any thoughts on this?
> >> > > > > > > >
> >> > > > > >
> >> > > > > > I think the current pg_is_wal_replay_paused() already has another purpose;
> >> > > > > > this waits recovery to actually get paused. If we want to limit this API's
> >> > > > > > purpose only to return the pause state, it seems better to fix this to return
> >> > > > > > the actual state at the cost of lacking the backward compatibility. If we want
> >> > > > > > to know whether pause is requested, we may add a new API like
> >> > > > > > pg_is_wal_replay_paluse_requeseted(). Also, if we want to wait recovery to actually
> >> > > > > > get paused, we may add an option to pg_wal_replay_pause() for this purpose.
> >> > > > > >
> >> > > > > > However, this might be a bikeshedding. If anyone don't care that
> >> > > > > > pg_is_wal_replay_paused() can make user wait for a long time, I don't care either.
> >> > > > >
> >> > > > > I don't think that it will be blocked ever, because
> >> > > > > pg_wal_replay_pause is sending the WakeupRecovery() which means the
> >> > > > > recovery process will not be stuck on waiting for the WAL.
> >> > >
> >> > > Yes, there is no stuck on waiting for the WAL. However, it can be stuck during resolving
> >> > > a recovery conflict. The process could wait for max_standby_streaming_delay or
> >> > > max_standby_archive_delay at most before recovery get completely paused.
> >> >
> >> > Okay, I agree that it is possible so for handling this we have a
> >> > couple of options
> >> > 1. pg_is_wal_replay_paused(), interface will wait for recovery to
> >> > actually get paused, but user have an option to cancel that.  So I
> >> > agree that there is currently no option to just know that recovery
> >> > pause is requested without waiting for its actually get paused if it
> >> > is requested.  So one option is we can provide an another interface as
> >> > you mentioned pg_is_wal_replay_paluse_requeseted(), which can just
> >> > return the request status.  I am not sure how useful it is.
> >>
> >> If it is acceptable that pg_is_wal_replay_paused() makes users wait,
> >> I'm ok for the current interface. I don't feel the need of
> >> pg_is_wal_replay_paluse_requeseted().
> >>
> >> >
> >> > 2. Pass an option to pg_is_wal_replay_paused whether to wait for
> >> > recovery to actually get paused or not.
> >> >
> >> > 3. Pass an option to pg_wal_replay_pause(), whether to wait for
> >> > recovery pause or just request and return.
> >> >
> >> > I like the option 1, any other opinion on this?
> >> >
> >> > > Also, it could wait for recovery_min_apply_delay if it has a valid value. It is possible
> >> > > that a user set this parameter to a large value, so it could wait for a long time. However,
> >> > > this will be avoided by calling recoveryPausesHere() or CheckAndSetRecoveryPause() in
> >> > > recoveryApplyDelay().
> >> >
> >> > Right
> >>
> >> Is there any reason not to do it?
> >
> >
> >
> > I think I missed that.. I will do in the next version
> >
> 
> In the last patch there were some local changes which I did not add to
> the patch and it was giving compilation warning so fixed that along
> with that I have addressed your this comment as well.

Thank you fixing this!

I noticed that, after this fix, the following recoveryPausesHere() might
be unnecessary because this test and pause is already done in recoveryApplyDelay
 What do you think about it?

                if (recoveryApplyDelay(xlogreader))
                {
                    /*
                     * We test for paused recovery again here. If user sets
                     * delayed apply, it may be because they expect to pause
                     * recovery in case of problems, so we must test again
                     * here otherwise pausing during the delay-wait wouldn't
                     * work.
                     */
                    if (((volatile XLogCtlData *) XLogCtl)->recoveryPauseState)
                        recoveryPausesHere(false);
                }

Regards,
Yugo Nagata
-- 
Yugo NAGATA <nagata@sraoss.co.jp>