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
