Subject: Re: New WAL record to detect the checkpoint redo location

From: Amit Kapila <[email-masked]>
Date: Thu, 21 Sep 2023 13:52:34 +0530

On Thu, Sep 21, 2023 at 7:05 AM Robert Haas <[email-masked]> wrote:
>
> On Mon, Sep 18, 2023 at 2:57 PM Robert Haas <[email-masked]> 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.

---

From: Robert Haas <[email-masked]>
Date: Thu, 21 Sep 2023 11:36:41 -0400

> 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.

> 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

Good to know, thanks.

> 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