Re: [Patch] ALTER SYSTEM READ ONLY

On Thu, Sep 9, 2021 at 11:12 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
>
>

Thank you, for looking at the patch.  Please see my reply inline below:

>
> > On Sep 8, 2021, at 6:44 AM, Amul Sul <sulamul@gmail.com> wrote:
> >
> > Here is the rebased version.
>
> v33-0004
>
> This patch moves the include of "catalog/pg_control.h" from transam/xlog.c into access/xlog.h, making pg_control.h
indirectlyincluded from a much larger set of files.  Maybe that's ok.  I don't know.  But it seems you are doing this
merelyto get the symbol (not even the definition) for struct DBState.  I'd recommend rearranging the code so this isn't
necessary,but otherwise you'd at least want to remove the now redundant includes of catalog/pg_control.h from
xlogdesc.c,xloginsert.c, auth-scram.c, postmaster.c, misc/pg_controldata.c, and pg_controldata/pg_controldata.c. 
>

Yes, you are correct, xlog.h is included in more than 150 files. I was
wondering if we can have a forward declaration instead of including
pg_control.h (e.g. The same way struct XLogRecData was declared in
xlog.h). Perhaps, DBState is enum & I don't see we have done the same
for enum elsewhere as we are doing for structures, but that seems to
be fine, IMO.

Earlier, I was unsure before preparing this patch, but since that
makes sense (I assumed) and minimizes duplications, can we go ahead
and post separately with the same change in StartupXLOG() which I have
skipped for the same reason mentioned in patch commit-msg.

> v33-0005
>
> This patch makes bool XLogInsertAllowed() more complicated than before.  The result used to depend mostly on the
valueof LocalXLogInsertAllowed except that when that value was negative, the result was determined by
RecoveryInProgress(). There was an arcane rule that LocalXLogInsertAllowed must have the non-negative values binary
coercibleto boolean "true" and "false", with the basis for that rule being the coding of XLogInsertAllowed().  Now that
thefunction is more complicated, this rule seems even more arcane.  Can we change the logic to not depend on casting an
integerto bool? 
>

We can't use a boolean variable because LocalXLogInsertAllowed
represents three states as, 1 means "wal is allowed'', 0 for "wal is
disallowed", and -1 is for "need to check".

> The code comment change in autovacuum.c introduces a non-grammatical sentence: "First, the system is not read only
i.e.wal writes permitted". 
>
> The function comment in checkpointer.c reads more like it toggles the system into allowing something, rather than
actuallydoing that same something: "SendSignalToCheckpointer allows a process to send a signal to the checkpoint
process".
>
> The new code comment in ipci.c contains a typo, but more importantly, it doesn't impart any knowledge beyond what a
readerof the function name could already surmise.  Perhaps the comment can better clarify what is happening: "Set up
walprobibit shared state" 
>
> The new code comment in sync.c copies and changes a nearby comment but drops part of the verb phrase:  "As in
ProcessSyncRequests,we don't want to stop wal prohibit change requests".  The nearby comment reads "stop absorbing".  I
thinkthis one should read "stop processing".  This same comment is used again below.   Then a third comment reads "For
thesame reason mentioned previously for the wal prohibit state change request check."  That third comment is too glib. 
>
> tcop/utility.c needlessly includes "access/walprohibit.h"
>
> wait_event.h extends enum WaitEventIO with new values WAIT_EVENT_WALPROHIBIT_STATE and
WAIT_EVENT_WALPROHIBIT_STATE_CHANGE. I don't find the difference between these two names at all clear.  Waiting for a
statechange is clear enough.  But how is waiting on a state different? 
>
> xlog.h defines a new enum.  I don't find any of it clear; not the comment, nor the name of the enum, nor the names of
thevalues: 
>
> /* State of work that enables wal writes */
> typedef enum XLogAcceptWritesState
> {
>     XLOG_ACCEPT_WRITES_PENDING = 0, /* initial state, not started */
>     XLOG_ACCEPT_WRITES_SKIPPED,     /* skipped wal writes */
>     XLOG_ACCEPT_WRITES_DONE         /* wal writes are enabled */
> } XLogAcceptWritesState;
>
> This enum seems to have been written from the point of view of someone who already knew what it was for.  It needs to
bewritten in a way that will be clear to people who have no idea what it is for. 
>
> v33-0006:
>
> The new code comments in brin.c and elsewhere should use the verb "require" rather than "have", otherwise "building
indexes"reads as a noun phrase rather than as a gerund: /* Building indexes will have an XID */ 
>

Will try to think about the pointed code comments for the improvements.

> The new function CheckWALPermitted() seems to test the current state of variables but not lock any of them, and the
newfunction comment says: 
>

CheckWALPermitted() calls XLogInsertAllowed() does check the
LocalXLogInsertAllowed flag which is local to that process only, and
nobody else reads that concurrently.

> /*
>  * In opposite to the above assertion if a transaction doesn't have valid XID
>  * (e.g. VACUUM) then it won't be killed while changing the system state to WAL
>  * prohibited.  Therefore, we need to explicitly error out before entering into
>  * the critical section.
>  */
>
> This suggests to me that a vacuum process can check whether wal is prohibited, then begin a critical section which
needswal to be allowed, and concurrently somebody else might disable wal without killing the vacuum process.  I'm given
towonder what horrors await when the vacuum process does something that needs to be wal logged but cannot be.  Does it
triggera panic?  I don't like the idea that calling pg_prohibit_wal durning a vacuum might panic the cluster.  If there
issome reason this is not a problem, I think the comment should explain it.  In particular, why is it sufficient to
checkwhether wal is prohibited before entering the critical section and not necessary to be sure it remains allowed
throughthe lifetime of that critical section? 
>

Hm, interrupts absorption are disabled inside the critical section.
The wal prohibited state for that process (here vacuum) will never get
set until it sees the interrupts & the system will not be said wal
prohibited until every process sees that interrupts. I am not sure we
should explain the characteristics of the critical section at this
place, if want, we can add a brief saying that inside the critical
section we should not worry about the state change which never happens
because interrupts are disabled there.

> v33-0007:
>
> I don't really like what the documentation has to say about pg_prohibit_wal.  Why should pg_prohibit_wal differ from
othersignal sending functions in whether it returns a boolean?  If you believe it must always succeed, you can still
defineit as returning a boolean and always return true.  That leaves the door open to future code changes which might
needto return false for some reason. 
>

Ok, I am fine to always return true.

> But I also don't like the idea that existing transactions with xids are immediately killed.  Shouldn't this function
takean optional timeout, perhaps defaulting to none, but otherwise allowing the user to put the system into
WALPROHIBIT_STATE_GOING_READ_ONLYfor a period of time before killing remaining transactions? 
>

Ok, will check.

> Why is this function defined to take a boolean such that pg_prohibit_wal(true) means to prohibit wal and
pg_prohibit_wal(false)means to allow wal.  Wouldn't a different function named pg_allow_wal() make it more clear?  This
alsowould be a better interface if taking the system read-only had a timeout as I suggested above, as such a timeout
parameterwhen allowing wal is less clearly useful. 
>

Like Robert, I am too inclined to have a single function that is easy
to remember. Apart from this, recently while testing this patch with
pgbench where I have exhausted the connection limit and want to change
the system's prohibited state in between but I was unable to do that,
I wish I could do that using the pg_clt option. How about having a
pg_clt option to alter wal prohibited state?

> That's enough code review for now.  Next I will review your regression tests....
>
Thanks again.