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.
>
> 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.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com