On Mon, May 17, 2021 at 11:48 AM Amul Sul <sula...@gmail.com> wrote: > > On Sat, May 15, 2021 at 3:12 PM Dilip Kumar <dilipbal...@gmail.com> wrote: > > > > On Thu, May 13, 2021 at 2:56 PM Dilip Kumar <dilipbal...@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
