Re: [Patch] ALTER SYSTEM READ ONLY

From: Amul Sul <sulamul(at)gmail(dot)com>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Andres Freund <andres(at)anarazel(dot)de>, Michael Paquier <michael(at)paquier(dot)xyz>, tushar <tushar(dot)ahuja(at)enterprisedb(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [Patch] ALTER SYSTEM READ ONLY
Date: 2020-07-29 10:35:00
Message-ID: CAAJ_b94dR7NKwnUmBi3bv99N_rqWHTtasSVGky7W89BEJk5q8w@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi,

The attached version is updated w.r.t. some of the review comments
from Soumyadeep and Andres.

Two thing from Andres' review comment are not addressed are:

1. Only superuser allowed to execute AlterSystemSetWALProhibitState(). As
per
Andres instead we should do this in a GRANTable manner. I tried that but
got a little confused with the roles that we could use for ASRO and didn't
see
any much appropriate one. pg_signal_backend could have been suited for ASRO
where we terminate some of the backends but a user granted this role is not
supposed to terminate the superuser backend. If we used that we need to
check a
superuser backend and raise an error or warning. Other roles are
pg_write_server_files or pg_execute_server_program but I am not sure we
should
use either of this, seems a bit confusing to me. Any suggestion or am I
missing
something here?

2. About walprohibit.c/.h file, Andres' concern on file name is that WAL
related file names are started with xlog. I think renaming to xlog* will
not be
the correct and will be more confusing since function/variable/macros inside
walprohibit.c/.h files contain the walprohibit keyword. And another
concern is due to
separate file we have to include it to many places but I think that will be
one time pain and worth it to keep code modularised.

Andres, Robert, do let me know your opinion on this if you think we should
merge
walprohibit.c/.h file into xlog.c/.h, will do that in the next version.

Changes in the attached version are:

1. Renamed readonly_cv to walprohibit_cv.
2. Removed repetitive comments for CheckWALPermitted() &
AssertWALPermitted_HaveXID().
3. Renamed AssertWALPermitted_HaveXID() to AssertWALPermittedHaveXID().
4. Changes to avoid repeated RelationNeedsWAL() calls.
5. IsWALProhibited() made static inline function.
6. Added outfuncs and readfuncs functions.
7. Added error when read-only state transition is in progress and other
backends
trying to make the system read-write or vice versa. Previously 2nd backend
seeing
command that was executed successfully but it wasn't.
8. Merged checkpointer code changes patch to 0002.

Regards,
Amul

Attachment Content-Type Size
v4-0001-Allow-error-or-refusal-while-absorbing-barriers.patch application/x-patch 4.4 KB
v4-0002-Add-alter-system-read-only-write-syntax.patch application/x-patch 9.3 KB
v4-0004-Error-or-Assert-before-START_CRIT_SECTION-for-WAL.patch application/x-patch 70.4 KB
v4-0005-Documentation-WIP.patch application/x-patch 6.5 KB
v4-0003-Implement-ALTER-SYSTEM-READ-ONLY-using-global-bar.patch application/x-patch 34.1 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Amul Sul 2020-07-29 11:05:08 Re: [Patch] ALTER SYSTEM READ ONLY
Previous Message Masahiko Sawada 2020-07-29 09:41:16 Re: [PATCH] Tab completion for VACUUM of partitioned tables