Hi, On 2023-02-10 18:38:50 +0200, Heikki Linnakangas wrote: > I'll continue reviewing this, but here's some feedback on the first two > patches: > > v2-0001-aio-Add-some-error-checking-around-pinning.patch: > > I wonder if the extra assertion in LockBufHdr() is worth the overhead. It > won't add anything without assertions, of course, but still. No objections > if you think it's worth it.
It's so easy to get confused about local/non-local buffers, that I think it is useful. I think we really need to consider cleaning up the separation further. Having half the code for local buffers in bufmgr.c and the other half in localbuf.c, without a scheme that I can recognize, is not a good scheme. It bothers me somewhat ConditionalLockBufferForCleanup() silently accepts multiple pins by the current backend. That's the right thing for e.g. heap_page_prune_opt(), but for something like lazy_scan_heap() it's not. And yes, I did encounter a bug hidden by that when making vacuumlazy use AIO as part of that patchset. That's why I made BufferCheckOneLocalPin() externally visible. > v2-0002-hio-Release-extension-lock-before-initializing-pa.patch: > > Looks as far as it goes. It's a bit silly that we use RBM_ZERO_AND_LOCK, > which zeroes the page, and then we call PageInit to zero the page again. > RBM_ZERO_AND_LOCK only zeroes the page if it wasn't in the buffer cache > previously, but with P_NEW, that is always true. It is quite silly, and it shows up noticably in profiles. The zeroing is definitely needed in other places calling PageInit(), though. I suspect we should have a PageInitZeroed() or such, that asserts the page is zero, but otherwise skips it. Seems independent enough from this series, that I'd probably tackle it separately? If you prefer, I'm ok with adding a patch to this series instead, though. Greetings, Andres Freund
