Re: [Patch] ALTER SYSTEM READ ONLY

Attached is rebase for the latest master head.  Also, I added one more
refactoring code that deduplicates the code setting database state in the
control file. The same code set the database state is also needed for this
feature.

Regards.
Amul

On Mon, May 17, 2021 at 1:07 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Mon, May 17, 2021 at 11:48 AM Amul Sul <sulamul@gmail.com> wrote:
> >
> > 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.
>
> 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