On Thu, Sep 21, 2023 at 4:22 AM Amit Kapila <amit.kapil...@gmail.com> wrote: > After the 0003 patch, do we need acquire exclusive lock via > WALInsertLockAcquireExclusive() for non-shutdown checkpoints. Even the > comment "We must block concurrent insertions while examining insert > state to determine the checkpoint REDO pointer." seems to indicate > that it is not required. If it is required then we may want to change > the comments and also acquiring the locks twice will have more cost > than acquiring it once and write the new WAL record under that lock.
I think the comment needs updating. I don't think we can do curInsert = XLogBytePosToRecPtr(Insert->CurrBytePos) without taking the locks. Same for Insert->fullPageWrites. I agree that it looks a little wasteful to release the lock and then reacquire it, but I suppose checkpoints don't happen often enough for it to matter. You're not going to notice an extra set of insertion lock acquisitions once every 5 minutes, or every half hour, or even every 1 minute if your checkpoints are super-frequent. Also notice that the current code is also quite inefficient in this way. GetLastImportantRecPtr() acquires and releases each lock one at a time, and then we immediately turn around and do WALInsertLockAcquireExclusive(). If the overhead that you're concerned about here were big enough to matter, we could reclaim what we're losing by having a version of GetLastImportantRecPtr() that expects to be called with all locks already held. But when I asked Andres, he thought that it didn't matter, and I bet he's right. > One minor comment: > + else if (XLOG_CHECKPOINT_REDO) > + class = WALINSERT_SPECIAL_CHECKPOINT; > + } > > Isn't the check needs to compare the record type with info? Yeah wow. That's a big mistake. > Your v6-0001* patch looks like an improvement to me even without the > other two patches. Good to know, thanks. > BTW, I would like to mention that there is a slight interaction of > this work with the patch to upgrade/migrate slots [1]. Basically in > [1], to allow slots migration from lower to higher version, we need to > ensure that all the WAL has been consumed by the slots before clean > shutdown. However, during upgrade we can generate few records like > checkpoint which we will ignore for the slot consistency checking as > such records doesn't matter for data consistency after upgrade. We > probably need to add this record to that list. I'll keep an eye on > both the patches so that we don't miss that interaction but mentioned > it here to make others also aware of the same. If your approach requires a code change every time someone adds a new WAL record that doesn't modify table data, you might want to rethink the approach a bit. -- Robert Haas EDB: http://www.enterprisedb.com
