On Fri, Sep 9, 2022 at 3:32 PM Dilip Kumar <dilipbal...@gmail.com> wrote: > > On Thu, Sep 8, 2022 at 4:10 PM Dilip Kumar <dilipbal...@gmail.com> wrote: > > > On a separate note, while reviewing the latest patch I see there is some > > risk of using the unflushed relfilenumber in GetNewRelFileNumber() > > function. Basically, in the current code, the flushing logic is tightly > > coupled with the logging new relfilenumber logic and that might not work > > with all the values of the VAR_RELNUMBER_NEW_XLOG_THRESHOLD. So the idea > > is we need to keep the flushing logic separate from the logging, I am > > working on the idea and I will post the patch soon. > > I have fixed the issue, so now we will track nextRelFileNumber, > loggedRelFileNumber and flushedRelFileNumber. So whenever > nextRelFileNumber is just VAR_RELNUMBER_NEW_XLOG_THRESHOLD behind the > loggedRelFileNumber we will log VAR_RELNUMBER_PER_XLOG more > relfilenumbers. And whenever nextRelFileNumber reaches the > flushedRelFileNumber then we will do XlogFlush for WAL upto the last > loggedRelFileNumber. Ideally flushedRelFileNumber should always be > VAR_RELNUMBER_PER_XLOG number behind the loggedRelFileNumber so we can > avoid tracking the flushedRelFileNumber. But I feel keeping track of > the flushedRelFileNumber looks cleaner and easier to understand. For > more details refer to the code in GetNewRelFileNumber(). >
Here are a few minor suggestions I came across while reading this patch, might be useful: +#ifdef USE_ASSERT_CHECKING + + { Unnecessary space after USE_ASSERT_CHECKING. -- + return InvalidRelFileNumber; /* placate compiler */ I don't think we needed this after the error on the latest branches. -- + LWLockAcquire(RelFileNumberGenLock, LW_SHARED); + if (shutdown) + checkPoint.nextRelFileNumber = ShmemVariableCache->nextRelFileNumber; + else + checkPoint.nextRelFileNumber = ShmemVariableCache->loggedRelFileNumber; + + LWLockRelease(RelFileNumberGenLock); This is done for the good reason, I think, it should have a comment describing different checkPoint.nextRelFileNumber assignment need and crash recovery perspective. -- +#define SizeOfRelFileLocatorBackend \ + (offsetof(RelFileLocatorBackend, backend) + sizeof(BackendId)) Can append empty parenthesis "()" to the macro name, to look like a function call at use or change the macro name to uppercase? -- + if (val < 0 || val > MAX_RELFILENUMBER) .. if ((relfilenumber) < 0 || (relfilenumber) > MAX_RELFILENUMBER) \ How about adding a macro for this condition as RelFileNumberIsValid()? We can replace all the checks referring to MAX_RELFILENUMBER with this. Regards, Amul