Re: Is Recovery actually paused?

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.

> >
> > + /* 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);

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center