Re: [Patch] ALTER SYSTEM READ ONLY

On Thu, Jul 23, 2020 at 6:08 AM Soumyadeep Chakraborty
<soumyadeep2007@gmail.com> wrote:
>
> Hi Amul,
>

Thanks, Soumyadeep for looking and putting your thoughts on the patch.

> On Tue, Jun 16, 2020 at 6:56 AM amul sul <sulamul@gmail.com> wrote:
> > The proposed feature is built atop of super barrier mechanism commit[1] to
> > coordinate
> > global state changes to all active backends.  Backends which executed
> > ALTER SYSTEM READ { ONLY | WRITE } command places request to checkpointer
> > process to change the requested WAL read/write state aka WAL prohibited and
> > WAL
> > permitted state respectively.  When the checkpointer process sees the WAL
> > prohibit
> > state change request, it emits a global barrier and waits until all
> > backends that
> > participate in the ProcSignal absorbs it.
>
> Why should the checkpointer have the responsibility of setting the state
> of the system to read-only? Maybe this should be the postmaster's
> responsibility - the checkpointer should just handle requests to
> checkpoint.

Well, once we've initiated the change to a read-only state, we probably want to
always either finish that change or go back to read-write, even if the process
that initiated the change is interrupted. Leaving the system in a
half-way-in-between state long term seems bad. Maybe we would have put some
background process, but choose the checkpointer in charge of making the state
change and to avoid the new background process to keep the first version patch
simple.  The checkpointer isn't likely to get killed, but if it does, it will
be relaunched and the new one can clean things up. On the other hand, I agree
making the checkpointer responsible for more than one thing might not
be a good idea
but I don't think the postmaster should do the work that any
background process can
do.

>I think the backend requesting the read-only transition
> should signal the postmaster, which in turn, will take on the aforesaid
> responsibilities. The postmaster, could also additionally request a
> checkpoint, using RequestCheckpoint() (if we want to support the
> read-onlyness discussed in [1]). checkpointer.c should not be touched by
> this feature.
>
> Following on, any condition variable used by the backend to wait for the
> ALTER SYSTEM command to finish (the patch uses
> CheckpointerShmem->readonly_cv), could be housed in ProcGlobal.
>

Relevant only if we don't want to use the checkpointer process.

Regards,
Amul