Re: [Patch] ALTER SYSTEM READ ONLY

On Sat, May 15, 2021 at 3:12 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Thu, May 13, 2021 at 2:56 PM Dilip Kumar <dilipbalaut@gmail.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.
>

I understand that we should not have an error inside a critical section but
this check is not wrong. Patch has enough checking so that errors due to WAL
prohibited state must not hit in the critical section, see assert just before
CheckWALPermitted().  Before entering into the critical section, we do have an
explicit WAL prohibited check. And to make sure that check has been done for
all current critical section for the wal writes, we have aforesaid assert
checking, for more detail on this please have a look at the "WAL prohibited
system state" section of src/backend/access/transam/README added in 0004 patch.
This assertion also ensures that future development does not miss the WAL
prohibited state check before entering into a newly added critical section for
WAL writes.

> ===
>
> +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
>
> ===

Fixed all in the attached version.

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

Thanks for the review.

Regards,
Amul