Re: [Patch] ALTER SYSTEM READ ONLY

On Tue, May 11, 2021 at 4:50 PM Amul Sul <sulamul(at)gmail(dot)com> wrote:
>
> On Tue, May 11, 2021 at 4:13 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > I might be missing something, but assume the behavior should be like this
> >
> > 1. If the state is getting changed from WALPROHIBIT_STATE_READ_WRITE
> > -> WALPROHIBIT_STATE_READ_ONLY, then as soon as the backend process
> > the barrier, we can immediately abort any read-write transaction(and
> > stop allowing WAL writing), because once we ensure that all session
> > has responded that now they have no read-write transaction then we can
> > safely change the state from WALPROHIBIT_STATE_GOING_READ_ONLY to
> > WALPROHIBIT_STATE_READ_ONLY.
> >
>
> Yes, that's what the current patch is doing from the first patch version.
>
> > 2. OTOH, if we are changing from WALPROHIBIT_STATE_READ_ONLY ->
> > WALPROHIBIT_STATE_READ_WRITE, then we don't need to allow the backend
> > to consider the system as read-write, instead, we should wait until
> > the shared state is changed to WALPROHIBIT_STATE_READ_WRITE.
> >
>
> I am sure that only not enough will have the same issue where
> LocalXLogInsertAllowed gets set the same as the read-only as described in
> my previous reply.

Okay, but while browsing the code I do not see any direct if condition
based on the "LocalXLogInsertAllowed" variable, can you point me to
some references?
I only see one if check on this variable and that is in
XLogInsertAllowed() function, but now in XLogInsertAllowed() function,
you are already checking IsWALProhibited. No?

> > Other than this point, I think the state names READ_ONLY, READ_WRITE
> > are a bit confusing no? because actually, these states represent
> > whether WAL is allowed or not, but READ_ONLY, READ_WRITE seems like we
> > are putting the system under a Read-only state. For example, if you
> > are doing some write operation on an unlogged table will be allowed, I
> > guess because that will not generate the WAL until you commit (because
> > commit generates WAL) right? so practically, we are just blocking the
> > WAL, not the write operation.
> >
>
> This read-only and read-write are the wal prohibited states though we
> are using for read-only/read-write system in the discussion and the
> complete macro name is WALPROHIBIT_STATE_READ_ONLY and
> WALPROHIBIT_STATE_READ_WRITE, I am not sure why that would make
> implementation confusing.

Fine, I am not too particular about these names.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com