Re: [Patch] ALTER SYSTEM READ ONLY

On Thu, Sep 9, 2021 at 1:42 PM Mark Dilger <mark(dot)dilger(at)enterprisedb(dot)com> wrote:
> 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 */

Honestly that sentence doesn't sound very clear even with a different verb.

> This suggests to me that a vacuum process can check whether wal is prohibited, then begin a critical section which needs wal to be allowed, and concurrently somebody else might disable wal without killing the vacuum process. I'm given to wonder what horrors await when the vacuum process does something that needs to be wal logged but cannot be. Does it trigger a panic? I don't like the idea that calling pg_prohibit_wal durning a vacuum might panic the cluster. If there is some reason this is not a problem, I think the comment should explain it. In particular, why is it sufficient to check whether wal is prohibited before entering the critical section and not necessary to be sure it remains allowed through the lifetime of that critical section?

The idea here is that if a transaction already has an XID assigned, we
have to kill it off before we can declare the system read-only,
because it will definitely write WAL when the transaction ends: either
a commit record, or an abort record, but definitely something. So
cases where we write WAL without necessarily having an XID require
special handling. They have to check whether WAL has become prohibited
and error out if so, and they need to do so before entering the
critical section - because if the problem were detected for the first
time inside the critical section it would escalate to a PANIC, which
we do not want. Places where we're guaranteed to have an XID - e.g.
inserting a heap tuple - don't need a run-time check before entering
the critical section, because the code can't be reached in the first
place if the system is WAL-read-only.

> 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 also would be a better interface if taking the system read-only had a timeout as I suggested above, as such a timeout parameter when allowing wal is less clearly useful.

Hmm, I find pg_prohibit_wal(true/false) better than pg_prohibit_wal()
and pg_allow_wal(), and would prefer pg_prohibit_wal(true/false,
timeout) over pg_prohibit_wal(timeout) and pg_allow_wal(), because I
think then once you find that one function you know how to do
everything about that feature, whereas the other way you need to find
both functions to have the whole story. That said, I can see why
somebody else might prefer something else.

--
Robert Haas
EDB: http://www.enterprisedb.com