Re: [Patch] ALTER SYSTEM READ ONLY

On Fri, Jul 24, 2020 at 10:40 PM Soumyadeep Chakraborty <
soumyadeep2007(at)gmail(dot)com> wrote:

> On Thu, Jul 23, 2020 at 10:14 PM Amul Sul <sulamul(at)gmail(dot)com> wrote:
> >
> > On Fri, Jul 24, 2020 at 6:28 AM Soumyadeep Chakraborty <
> soumyadeep2007(at)gmail(dot)com> wrote:
> > > In case it is necessary, the patch set does not wait for the
> checkpoint to
> > > complete before marking the system as read-write. Refer:
> > >
> > > /* Set final state by clearing in-progress flag bit */
> > > if (SetWALProhibitState(wal_state &
> > ~(WALPROHIBIT_TRANSITION_IN_PROGRESS)))
> > > {
> > > if ((wal_state & WALPROHIBIT_STATE_READ_ONLY) != 0)
> > > ereport(LOG, (errmsg("system is now read only")));
> > > else
> > > {
> > > /* Request checkpoint */
> > > RequestCheckpoint(CHECKPOINT_IMMEDIATE);
> > > ereport(LOG, (errmsg("system is now read write")));
> > > }
> > > }
> > >
> > > We should RequestCheckpoint(CHECKPOINT_IMMEDIATE | CHECKPOINT_WAIT)
> before
> > > we SetWALProhibitState() and do the ereport(), if we have a read-write
> > > state change request.
> > >
> > +1, I too have the same question.
> >
> >
> >
> > FWIW, I don't we can request CHECKPOINT_WAIT for this place, otherwise,
> it
> > think
> > it will be deadlock case -- checkpointer process waiting for itself.
>
> We should really just call CreateCheckPoint() here instead of
> RequestCheckpoint().
>
>
The only setting flag would have been enough for now, the next loop of
CheckpointerMain() will anyway be going to call CreateCheckPoint() without
waiting. I used RequestCheckpoint() to avoid duplicate flag setting code.
Also, I think RequestCheckpoint() will be better so that we don't need to
deal
will the standalone backend, the only imperfection is it will unnecessary
signal
itself, that would be fine I guess.

> > 3. Some of the functions that were added such as GetWALProhibitState(),
> > > IsWALProhibited() etc could be declared static inline.
> > >
> > IsWALProhibited() can be static but not GetWALProhibitState() since it
> > needed to
> > be accessible from other files.
>
> If you place a static inline function in a header file, it will be
> accessible from other files. E.g. pg_atomic_* functions.
>

Well, the current patch set also has few inline functions in the header
file.
But, I don't think we can do the same for GetWALProhibitState() without
changing
the XLogCtl structure scope which is local to xlog.c file and the changing
XLogCtl
scope would be a bad idea.

Regards,
Amul