Re: [Patch] ALTER SYSTEM READ ONLY

On Sat, Apr 23, 2022 at 1:34 PM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> On Mon, Mar 15, 2021 at 12:56 PM Amul Sul <sulamul(at)gmail(dot)com> wrote:
> > >
> > > It is a very minor change, so I rebased the patch. Please take a look, if that works for you.
> > >
> >
> > Thanks, I am getting one more failure for the vacuumlazy.c. on the
> > latest master head(d75288fb27b), I fixed that in attached version.
>
> Thanks Amul! I haven't looked at the whole thread, I may be repeating
> things here, please bear with me.
>

Np, thanks for looking into it.

> 1) Is the pg_prohibit_wal() only user sets the wal prohibit mode? Or
> do we still allow via 'ALTER SYSTEM READ ONLY/READ WRITE'? If not, I
> think the patches still have ALTER SYSTEM READ ONLY references.

Could you please point me to what those references are? I didn't find
any in the v45 version.

> 2) IIUC, the idea of this patch is not to generate any new WAL when
> set as default_transaction_read_only and transaction_read_only can't
> guarantee that?

No. Complete WAL write should be disabled, in other words XLogInsert()
should be restricted.

> 3) IMO, the function name pg_prohibit_wal doesn't look good where it
> also allows one to set WAL writes, how about the following functions -
> pg_prohibit_wal or pg_disallow_wal_{generation, inserts} or
> pg_allow_wal or pg_allow_wal_{generation, inserts} without any
> arguments and if needed a common function
> pg_set_wal_generation_state(read-only/read-write) something like that?

There are already similar suggestions before too, but none of that
finalized yet, there are other more challenges that need to be
handled, so we can keep this work at last.

> 4) It looks like only the checkpointer is setting the WAL prohibit
> state? Is there a strong reason for that? Why can't the backend take a
> lock on prohibit state in shared memory and set it and let the
> checkpointer read it and block itself from writing WAL?

Once WAL prohibited state transition is initiated and should be
completed, there is no fallback. What if the backed exit before the
complete transition? Similarly, even if the checkpointer exits,
that will be restarted again and will complete the state transition.

> 5) Is SIGUSR1 (which is multiplexed) being sent without a "reason" to
> checkpointer? Why?

Simply want to wake up the checkpointer process without asking for
specific work in the handle function. Another suitable choice will be
SIGINT, we can choose that too if needed.

> 6) What happens for long-running or in-progress transactions if
> someone prohibits WAL in the midst of them? Do these txns fail? Or do
> we say that we will allow them to run to completion? Or do we fail
> those txns at commit time? One might use this feature to say not let
> server go out of disk space, but if we allow in-progress txns to
> generate/write WAL, then how can one achieve that with this feature?
> Say, I monitor my server in such a way that at 90% of disk space,
> prohibit WAL to avoid server crash. But if this feature allows
> in-progress txns to generate WAL, then the server may still crash?

Read-only transactions will be allowed to continue, and if that
transaction tries to write or any other transaction that has performed
any writes already then the session running that transaction will be
terminated -- the design is described in the first mail of this
thread.

> 7) What are the other use-cases (I can think of - to avoid out of disk
> crashes, block/freeze writes to database when the server is
> compromised) with this feature? Any usages during/before failover,
> promotion or after it?

The important use case is for failover to avoid split-brain situations.

> 8) Is there a strong reason that we've picked up conditional variable
> wal_prohibit_cv over mutex/lock for updating WALProhibit shared
> memory?

I am not sure how that can be done using mutex or lock.

> 9) Any tests that you are planning to add?

Yes, we can. I have added very sophisticated tests that cover most of
my code changes, but that is not enough for such critical code
changes, have a lot of chances of improvement and adding more tests
for this module as well as other parts e.g. some missing coverage of
gin, gists, brin, core features where this patch is adding checks, etc.
Any help will be greatly appreciated.

Regards,
Amul