Re: [Patch] ALTER SYSTEM READ ONLY

On Wed, May 12, 2021 at 11:09 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Tue, May 11, 2021 at 11:54 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> >
> > On Tue, May 11, 2021 at 11:17 AM Amul Sul <sulamul(at)gmail(dot)com> wrote:
> > > I think I have much easier solution than this, will post that with update version patch set tomorrow.
> >
> > I don't know what you have in mind, but based on this discussion, it
> > seems to me that we should just have 5 states instead of 4:
> >

I had to have two different ideas, the first one is a little bit
aligned with the approach you mentioned below but without introducing
a new state. Basically, what we want is to restrict any backend that
connects to the server and write a WAL record while we are doing
XLogAcceptWrites(). For XLogAcceptWrites() skip we do already have a
flag for that, when that flag is set (i.e. XLogAcceptWrites() skipped
previously) then treat the system as read-only (i.e. WAL prohibited)
until XLogAcceptWrites() finishes. In that case, our IsWALProhibited()
function will be:

bool
IsWALProhibited(void)
{
WALProhibitState cur_state;

/*
* If essential operations are needed to enable wal writes are skipped
* previously then treat this state as WAL prohibited until that gets
* done.
*/
if (unlikely(GetXLogWriteAllowedState() == XLOG_ACCEPT_WRITES_SKIPPED))
return true;

cur_state = GetWALProhibitState(GetWALProhibitCounter());

return (cur_state != WALPROHIBIT_STATE_READ_WRITE &&
cur_state != WALPROHIBIT_STATE_GOING_READ_WRITE);
}

Another idea that I want to propose & did the changes according to in
the attached version is making IsWALProhibited() something like this:

bool
IsWALProhibited(void)
{
/* Other than read-write state will be considered as read-only */
return (GetWALProhibitState(GetWALProhibitCounter()) !=
WALPROHIBIT_STATE_READ_WRITE);
}

But this needs some additional changes to CompleteWALProhibitChange()
function where the final in-memory system state update happens
differently i.e. before or after emitting a global barrier.

When in-memory WAL prohibited state is _GOING_READ_WRITE then
in-memory state immediately changes to _READ_WRITE. After that global
barrier is emitted for other backends to change their local state.
This should be harmless because a _READ_WRITE system could have
_READ_ONLY and _READ_WRITE backends.

But when the in-memory WAL prohibited state is _GOING_READ_ONLY then
in-memory update for the final state setting is not going to happen
before the global barrier. We cannot say the system is _READ_ONLY
until we ensure that all backends are _READ_ONLY.

For more details please have a look at CompleteWALProhibitChange().
Note that XLogAcceptWrites() happens before
CompleteWALProhibitChange() so if any backend connect while
XLogAcceptWrites() is in progress and will not allow WAL writes until it
gets finished and CompleteWALProhibitChange() executed.

The second approach is much better, IMO, because IsWALProhibited() is
much lighter which would run a number of times when a new backend
connects and/or its LocalXLogInsertAllowed cached value gets reset.
Perhaps, you could argue that the number of calls might not be that
much due to the locally cached value in LocalXLogInsertAllowed, but I
am in favour of having less work.

Apart from this, I made a separate patch for XLogAcceptWrites()
refactoring. Now, each patch can be compiled without having the next
patch on top of it.

> > 1. WAL is permitted.
> > 2. WAL is being prohibited but some backends may not know about the change yet.
> > 3. WAL is prohibited.
> > 4. WAL is in the process of being permitted but XLogAcceptWrites() may
> > not have been called yet.
> > 5. WAL is in the process of being permitted and XLogAcceptWrites() has
> > been called but some backends may not know about the change yet.
> >
> > If we're in state #3 and someone does pg_prohibit_wal(false) then we
> > enter state #4. The checkpointer calls XLogAcceptWrites(), moves us to
> > state #5, and pushes out a barrier. Then it waits for the barrier to
> > be absorbed and, when it has been, it moves us to state #1. Then if
> > someone does pg_prohibit_wal(true) we move to state #2. The
> > checkpointer pushes out a barrier and waits for it to be absorbed.
> > Then it calls XLogFlush() and afterward moves us to state #3.
> >
> > We can have any (reasonable) number of states that we want. There's
> > nothing magical about 4.
>
> Your idea makes sense, but IMHO, if we are first writing
> XLogAcceptWrites() and then pushing out the barrier, then I don't
> understand the meaning of having state #4. I mean whenever any
> backend receives the barrier the system will always be in state #5.
> So what do we want to do with state #4?
>
> Is it just to make the state machine better? I mean in the checkpoint
> process, we don't need separate "if checks" whether the
> XLogAcceptWrites() is called or not, instead we can just rely on the
> state, if it is #4 then we have to call XLogAcceptWrites(). If so
> then I think it's okay to have an additional state, just wanted to
> know what idea you had in mind?
>
AFAICU, that proposed state #4 is to restrict the newly connected
backend from WAL writes. My first approach doing the same by changing
IsWALProhibited() a bit.

Regards,
Amul