On Thu, 11 Feb 2021 16:36:55 +0530
Dilip Kumar <dilipbalaut@gmail.com> wrote:
> On Thu, Feb 11, 2021 at 3:20 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > On Wed, Feb 10, 2021 at 8:39 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > >
> > > On Wed, Feb 10, 2021 at 10:02 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > > >
> > > > I don't find any problem with this approach as well, but I personally
> > > > feel that the other approach where we don't wait in any API and just
> > > > return the recovery pause state is much simpler and more flexible. So
> > > > I will make the pending changes in that patch and let's see what are
> > > > the other opinion and based on that we can conclude. Thanks for the
> > > > patch.
I don't think that we need to include the waiting approach in pg_get_wal_replay_pause_state
patch. However, Horiguchi-san's patch may be useful for some users who want
pg_wal_replay_pause to wait until recovery gets paused instead of polling the
state from applications. So, I shink we could discuss this patch in another
thread as another commitfest entry independent from pg_get_wal_replay_pause_state.
> > > Here is an updated version of the patch which fixes the last two open problems
> > > 1. In RecoveryRequiresIntParameter set the recovery pause state in the
> > > loop so that if recovery resumed and pause requested again we can set
> > > to pause again.
> > > 2. If the recovery state is already 'paused' then don't set it back to
> > > the 'pause requested'.
> > >
> > > One more point is that in 'pg_wal_replay_pause' even if we don't
> > > change the state because it was already set to the 'paused' then also
> > > we call the WakeupRecovery. But I don't think there is any problem
> > > with that, if we think that this should be changed then we can make
> > > SetRecoveryPause return a bool such that if it doesn't do state change
> > > then it returns false and in that case we can avoid calling
> > > WakeupRecovery, but I felt that is unnecessary. Any other thoughts on
> > > this?
> >
> > IMO, that WakeupRecovery should not be a problem, because even now, if
> > we issue a simple select pg_reload_conf(); (without even changing any
> > config parameter), WakeupRecovery gets called.
> >
> > Thanks for the patch. I tested the new function and it works as
> > expected. I have no further comments on the v13 patch.
>
> Thanks for the review and testing.
I have no futher comments on the v13 patch, too. Also, I agree with
Robert Haas's suggestions.
Regards,
Yugo Nagata
--
Yugo NAGATA <nagata@sraoss.co.jp>
