On Thu, Jul 23, 2020 at 6:08 AM Soumyadeep Chakraborty <soumyadeep2...@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 <sula...@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