Re: [Patch] ALTER SYSTEM READ ONLY

On Mon, May 17, 2021 at 11:48 AM Amul Sul <sulamul(at)gmail(dot)com> wrote:
>
> On Sat, May 15, 2021 at 3:12 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > 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.
> >
>
> 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.

I think we need CheckWALPermitted(); check, in XLogBeginInsert()
function because if XLogBeginInsert() maybe called outside critical
section e.g. pg_truncate_visibility_map() then we should error out.
So this check make sense to me.

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