On Thu, Jul 23, 2020 at 10:14 PM Amul Sul <sulamul@gmail.com> wrote: > > On Fri, Jul 24, 2020 at 6:28 AM Soumyadeep Chakraborty <soumyadeep2007@gmail.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.