Re: Is Recovery actually paused?

At Wed, 24 Feb 2021 15:25:57 +0530, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote in
> On Wed, Feb 24, 2021 at 2:26 PM Kyotaro Horiguchi
> <horikyota(dot)ntt(at)gmail(dot)com> wrote:
> > 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.

Understood. I don't stick to the change.

> > > > 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.

Recently we have mildly changed to the direction to utilize the
compiler warning about enum coverage in switch struct. (Maybe we need
another compiler option that enables that check for switch'es with the
default case, though.) In that light, the direction is a switch
without the default case then Assert if none of the cases is stepped
on. This is what apply_dispatch does. Slightly different version of
the same would be the following. This is more natural than the above.

statestr = NULL;
swtich(state)
{
case RECOVERY_NOT_PAUSED:
statestr = "not paused";
break;
...
}

Assert (statestr != NULL);
return cstring_to_text(statestr);

If the enum had many (more than ten or so?) values and it didn't seem
stable I push that a bit strongly but it actually consists of only
three values and not likely to get further values. So I don't insist
on the style so strongly here.

In short, I'm fine with default: Assert(0) if you still don't like the
just above.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center