On Thu, Sep 21, 2023 at 7:05 AM Robert Haas <robertmh...@gmail.com> wrote: > > On Mon, Sep 18, 2023 at 2:57 PM Robert Haas <robertmh...@gmail.com> wrote: > > I've been brainstorming about this today, trying to figure out some > > ideas to make it work. > > Here are some patches. > > 0001 refactors XLogInsertRecord to unify a couple of separate tests of > isLogSwitch, hopefully making it cleaner and cheaper to add more > special cases. > > 0002 is a very minimal patch to add XLOG_CHECKPOINT_REDO without using > it for anything. > > 0003 modifies CreateCheckPoint() to insert an XLOG_CHECKPOINT_REDO > record for any non-shutdown checkpoint, and modifies > XLogInsertRecord() to treat that as a new special case, wherein after > inserting the record the redo pointer is reset while still holding the > WAL insertion locks. >
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. One minor comment: + else if (XLOG_CHECKPOINT_REDO) + class = WALINSERT_SPECIAL_CHECKPOINT; + } Isn't the check needs to compare the record type with info? Your v6-0001* patch looks like an improvement to me even without the other two patches. 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. [1] - https://www.postgresql.org/message-id/TYAPR01MB586615579356A84A8CF29A00F5F9A%40TYAPR01MB5866.jpnprd01.prod.outlook.com -- With Regards, Amit Kapila.
