Re: [Patch] ALTER SYSTEM READ ONLY

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

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

Regards,
Soumyadeep