Hi,
On 2025-12-02 19:47:35 -0500, Andres Freund wrote:
> On 2025-11-25 11:54:00 -0500, Andres Freund wrote:
> > Thanks a lot for that detailed review! A few questions and comments, before I
> > try to address the comments in the next version.
>
> Here's that new new version, with the following changes
>
> - Some more micro-optimizations, most importantly adding a commit that doesn't
> initialize the delay in LockBufHdr() unless needed. With those I don't see a
> consistent slowdown anymore (slight speedup on one workstation, slight
> slowdown on another, in an absurdly adverse workload)
>
> - Tried to address Melanie's feedback, with some exceptions (some noted below,
> but I also need to make another pass through the reviews)
>
> - re-implemented AssertNotCatalogBufferLock() in the new world
>
> - Substantially expanded comments around setting hint bits (in buffer/README,
> heapam_visibility.c and bufmgr.c)
>
> - split out the change to fsm_vacuum_page() to start to lock the page into is
> own commit
>
> - reordered patch series so that smaller changes are before the 64bit-state
> and "Implement buffer content locks independently of" commits, so they can
> be committed while we finish cleaning the later changes
>
> - I didn't invest much in cleaning up the later patches ("Don't copy pages
> while writing out" and "Make UnlockReleaseBuffer() more efficient") yet,
> wanted to focus on the earlier patches first
>
>
> Todo:
>
> - still need to rename ResOwnerReleaseBufferPin(). Wondering about what to
> rename ResourceOwnerDesc.name to. "buffer ownership" maybe? Not great...
>
> - gistkillitems() complaint by Melanie
>
> - amortize vs batch vs SetHintBits comment + SHB_* names
>
> - for the next version I'll remove the BATCHMVCC_FEWER_ARGS conditionals from
> 0010. I don't love needing BatchMVCCState but I don't really see an
> alternative, the performance difference is pretty persistent.
>
>
> Questions:
> - ForEachLWLockHeldByMe() and LWLockDisown() aren't used anymore, should we
> remove them?
I'm planning to work on committing 0001, 0002, 0003, 0008 soon-ish, unless
somebody sees a reason to hold off on that. After that I think 0005, 0006
would be next. I think 0004 is a clear improvement, but nobody has looked at
it yet...
For 0007, I wished ConditionalLockBuffer() accepted the lock level, there's no
point in waiting for the lock in the use case. I'm on the fence about whether
it's worth changing the ~12 users of ConditionalLockBuffer()...
Greetings,
Andres Freund
