Re: New WAL record to detect the checkpoint redo location

On Thu, Sep 21, 2023 at 4:22 AM Amit Kapila <amit.kapila16@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