Re: Is Recovery actually paused?

On Wed, Feb 24, 2021 at 2:26 PM Kyotaro Horiguchi
<horikyota(dot)ntt(at)gmail(dot)com> wrote:
>
> At Wed, 24 Feb 2021 13:15:27 +0530, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote in
> > On Wed, Feb 24, 2021 at 12:39 PM Kyotaro Horiguchi
> > <horikyota(dot)ntt(at)gmail(dot)com> wrote:
> > >
> > > At Tue, 23 Feb 2021 12:03:32 +0530, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote in
> > > > On Fri, Feb 12, 2021 at 3:26 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > > How about something like this?
> > >
> > > Request to pause recovery. Server actually stops recovery at a
> > > convenient time. This can take a few seconds after the request. If you
> > > need to strictly guarantee that no further database change will occur,
> > > you can check using pg_get_wal_replay_ause_state(). Note that
> > > pg_is_wal_replay_paused() may return true before recovery actually
> > > stops.
> >
> > I still think that for the user-facing documentation purpose the
> > current paragraph looks better.
>
> Ok.
>
> > > The patch adds two loops whth the following logic:
> > >
> > > while (GetRecoveryPauseState() != RECOVERY_NOT_PAUSED)
> > > {
> > > ...
> > > ConfirmRecoveryPaused();
> > > <wait>
> > > }
> > >
> > > After the renaming of the function, the following structure looks
> > > simpler and more natural.
> > >
> > > while (ConfirmRecoveryPaused())
> > > {
> > > ...
> > > <wait>
> > > }
> >
> > So do you mean that if the pause is requested ConfirmRecoveryPaused
> > will set it to paused and if it is not paused then it will return
> > false? With the current function name, I don't think that will look
> > clean maybe we should change the name to something like
> > CheckAndConfirmRecoveryPaused? Or I am fine with the way it is now.
> > Any other thoughts?
>
> I should have took the meaning of "confirm" wrongly. I took that as
> "somehow determine if the recovery is to be paused". If that reading
> is completely wrong, I don't mind either re-chaging the function name
> or leaving all it alone.

I am fine with leaving it the way it is unless someone feels that we
should change it.

> > >
> > > + /* test for recovery pause, if user has requested the pause */
> > > + if (((volatile XLogCtlData *) XLogCtl)->recoveryPauseState !=
> > >
> > > The reason for the checkpoint is to move to "paused" state in a
> > > reasonable time. I think we need to mention that reason rather than
> > > what is done here.
> >
> > I will do that.
>
> Thanks.
>
> > >
> > > + /* get the recovery pause state */
> > > + switch(GetRecoveryPauseState())
> > > + {
> > > + case RECOVERY_NOT_PAUSED:
> > > + state = "not paused";
> > > + break;
> > > ...
> > > + default:
> > > + elog(ERROR, "invalid recovery pause state");
> > >
> > > This disables the static enum coverage check and it is not likely to
> > > have a wrong value here, other than the case of shared memory
> > > corruption. So we can remove the default case
> > > here. pg_get_replication_slots() is going that direction and
> > > apply_dispatch() is taking a slightly different way. Anyway I think
> > > that we can take away the default case.
> >
> > So do you think we should put an assert(0) in the default case?
>
> No. Just removing the default in the switch. If the value comes from
> some other source typically from disk or user-interraction, the
> default is necessary, but, in the first place if we have other than
> the defined value there, it is a sign of something worse than
> ERROR. If we care about that case, we *could* do the same thing with
> apply_dispatch().
>
> switch (GetRecoveryPauseState())
> {
> case RECOVERY_NOT_PAUSED:
> return cstring_to_text("not paused");
> ..
> }
>
> /* we shouldn't reach here */
> Assert (0);

I think for such cases IMHO the preferred style for PostgreSQL is that
we add Assert(0) in the default case, at least it appeared to me that
way.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com