Re: [Patch] ALTER SYSTEM READ ONLY - Mailing list pgsql-hackers

> 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. 

v33-0005

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

The code comment change in autovacuum.c introduces a non-grammatical sentence: "First, the system is not read only i.e.
walwrites 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 */ 

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

/*
 * 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? 

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. 

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? 

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. 

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

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company