On Mon, Jan 25, 2021 at 6:12 AM Masahiko Sawada <sawada.m...@gmail.com> wrote:
>
> On Sun, Jan 17, 2021 at 5:19 PM Dilip Kumar <dilipbal...@gmail.com> wrote:
> >
> > On Sat, Jan 16, 2021 at 8:59 AM Masahiko Sawada <sawada.m...@gmail.com>
> > wrote:
> > >
> > > On Wed, Jan 13, 2021 at 9:20 PM Dilip Kumar <dilipbal...@gmail.com> wrote:
> > > >
> > > > On Wed, Jan 13, 2021 at 3:35 PM Dilip Kumar <dilipbal...@gmail.com>
> > > > wrote:
> > > > >
> > > > > On Wed, Jan 13, 2021 at 3:27 PM Yugo NAGATA <nag...@sraoss.co.jp>
> > > > > wrote:
> > > > > >
> > > > > > On Thu, 10 Dec 2020 11:25:23 +0530
> > > > > > Dilip Kumar <dilipbal...@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.
> > > > >
> > > > > > > > > As another comment, while pg_is_wal_replay_paused is
> > > > > > > > > blocking, I can not cancel
> > > > > > > > > the query. I think CHECK_FOR_INTERRUPTS() is necessary in the
> > > > > > > > > waiting loop.
> > > > > >
> > > > > > How about this fix? I think users may want to cancel
> > > > > > pg_is_wal_replay_paused() during
> > > > > > this is blocking.
> > > > >
> > > > > Yeah, we can do this. I will send the updated patch after putting
> > > > > some more thought into these comments. Thanks again for the feedback.
> > > > >
> > > >
> > > > Please find the updated patch.
> > >
> > > I've looked at the patch. Here are review comments:
> > >
> > > + /* Recovery pause state */
> > > + RecoveryPauseState recoveryPause;
> > >
> > > Now that the value can have tri-state, how about renaming it to
> > > recoveryPauseState?
> >
> > This makes sense to me.
> >
> > > ---
> > > bool
> > > RecoveryIsPaused(void)
> > > +{
> > > + bool recoveryPause;
> > > +
> > > + SpinLockAcquire(&XLogCtl->info_lck);
> > > + recoveryPause = (XLogCtl->recoveryPause == RECOVERY_PAUSED) ?
> > > true : false;
> > > + SpinLockRelease(&XLogCtl->info_lck);
> > > +
> > > + return recoveryPause;
> > > +}
> > > +
> > > +bool
> > > +RecoveryPauseRequested(void)
> > > {
> > > bool recoveryPause;
> > >
> > > SpinLockAcquire(&XLogCtl->info_lck);
> > > - recoveryPause = XLogCtl->recoveryPause;
> > > + recoveryPause = (XLogCtl->recoveryPause !=
> > > RECOVERY_IN_PROGRESS) ? true : false;
> > > SpinLockRelease(&XLogCtl->info_lck);
> > >
> > > return recoveryPause;
> > > }
> > >
> > > We can write like recoveryPause = (XLogCtl->recoveryPause ==
> > > RECOVERY_PAUSED);
> >
> > In RecoveryPauseRequested, we just want to know whether the pause is
> > requested or not, even if the pause requested and not yet pause then
> > also we want to return true. So how
> > recoveryPause = (XLogCtl->recoveryPause == RECOVERY_PAUSED) will work?
>
> Sorry for the late response.
>
> What I wanted to say is that the ternary operator is not necessary in
> those cases.
>
> The codes,
>
> recoveryPause = (XLogCtl->recoveryPause == RECOVERY_PAUSED) ? true : false;
> recoveryPause = (XLogCtl->recoveryPause != RECOVERY_IN_PROGRESS) ? true :
> false;
>
> are equivalent with,
>
> recoveryPause = (XLogCtl->recoveryPause == RECOVERY_PAUSED);
> recoveryPause = (XLogCtl->recoveryPause != RECOVERY_IN_PROGRESS);
>
> respectively.
>
Yeah absolutely correct. Will changes.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
