Horton

Re: refactoring relation extension and BufferAlloc(), faster COPY

Andres Freund <andres@anarazel.de>

View thread View on postgr.es
From: Andres Freund <andres@anarazel.de>
To: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: vignesh C <vignesh21@gmail.com>, pgsql-hackers@postgresql.org, Thomas Munro <thomas.munro@gmail.com>, Melanie Plageman <melanieplageman@gmail.com>, Yura Sokolov <y.sokolov@postgrespro.ru>, Robert Haas <robertmhaas@gmail.com>, David Rowley <dgrowleyml@gmail.com>
Date: 2023-02-11T22:20:14Z
Lists: pgsql-hackers 
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

Commits

  1. lwlock: Fix quadratic behavior with very long wait lists

  2. Support RBM_ZERO_AND_CLEANUP_LOCK in ExtendBufferedRelTo(), add tests

  3. Use ExtendBufferedRelTo() in XLogReadBufferExtended()

  4. hio: Use ExtendBufferedRelBy() to extend tables more efficiently

  5. heapam: Pass number of required pages to RelationGetBufferForTuple()

  6. Convert many uses of ReadBuffer[Extended](P_NEW) to ExtendBufferedRel()

  7. Use ExtendBufferedRelTo() in {vm,fsm}_extend()

  8. bufmgr: Introduce infrastructure for faster relation extension

  9. bufmgr: Support multiple in-progress IOs by using resowner

  10. bufmgr: Acquire and clean victim buffer separately

  11. bufmgr: Add Pin/UnpinLocalBuffer()

  12. bufmgr: Add some more error checking [infrastructure] around pinning

  13. Add smgrzeroextend(), FileZero(), FileFallocate()

  14. Don't initialize page in {vm,fsm}_extend(), not needed

  15. bufmgr: Remove buffer-write-dirty tracepoints

  16. hio: Release extension lock before initializing page / pinning VM

  17. bufmgr: Fix undefined behaviour with, unrealistically, large temp_buffers

  18. Fix memory leak and inefficiency in CREATE DATABASE ... STRATEGY WAL_LOG

  19. pgstat: Track more detailed relation IO statistics

  20. Use unnamed POSIX semaphores on Cygwin.

  21. Include RelFileLocator fields individually in BufferTag.