Re: [Patch] ALTER SYSTEM READ ONLY

On Thu, May 13, 2021 at 2:56 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> Great thanks. I will review the remaining patch soon.

I have reviewed v28-0003, and I have some comments on this.

===
@@ -126,9 +127,14 @@ XLogBeginInsert(void)
Assert(mainrdata_last == (XLogRecData *) &mainrdata_head);
Assert(mainrdata_len == 0);

+ /*
+ * WAL permission must have checked before entering the critical section.
+ * Otherwise, WAL prohibited error will force system panic.
+ */
+ Assert(walpermit_checked_state != WALPERMIT_UNCHECKED ||
!CritSectionCount);
+
/* cross-check on whether we should be here or not */
- if (!XLogInsertAllowed())
- elog(ERROR, "cannot make new WAL entries during recovery");
+ CheckWALPermitted();

We must not call CheckWALPermitted inside the critical section,
instead if we are here we must be sure that
WAL is permitted, so better put an assert. Even if that is ensured by
some other mean then also I don't
see any reason for calling this error generating function.

===

+CheckWALPermitted(void)
+{
+ if (!XLogInsertAllowed())
+ ereport(ERROR,
+ (errcode(ERRCODE_READ_ONLY_SQL_TRANSACTION),
+ errmsg("system is now read only")));
+

system is now read only -> wal is prohibited (in error message)

===

- * We can't write WAL in recovery mode, so there's no point trying to
+ * We can't write WAL during read-only mode, so there's no point trying to

during read-only mode -> if WAL is prohibited or WAL recovery in
progress (add recovery in progress and also modify read-only to wal
prohibited)

===

+ if (!XLogInsertAllowed())
{
GUC_check_errcode(ERRCODE_FEATURE_NOT_SUPPORTED);
- GUC_check_errmsg("cannot set transaction read-write mode
during recovery");
+ GUC_check_errmsg("cannot set transaction read-write mode
while system is read only");
return false;
}

system is read only -> WAL is prohibited

===

I think that's all, I have to say about 0003.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com