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: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Cc: 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>
Date: 2022-10-29T18:39:05Z
Lists: pgsql-hackers 
Hi,

On 2022-10-29 18:33:53 +0530, Bharath Rupireddy wrote:
> On Sat, Oct 29, 2022 at 8:24 AM Andres Freund <andres@anarazel.de> wrote:
> >
> > Hi,
> >
> > I'm working to extract independently useful bits from my AIO work, to reduce
> > the size of that patchset. This is one of those pieces.
> 
> Thanks a lot for this great work. There are 12 patches in this thread,
> I believe each of these patches is trying to solve separate problems
> and can be reviewed and get committed separately, am I correct?

Mostly, yes.

For 0001 I already started
https://www.postgresql.org/message-id/20221027165914.2hofzp4cvutj6gin%40awork3.anarazel.de
to discuss the specific issue.

We don't strictly need v1-0002-aio-Add-some-error-checking-around-pinning.patch
but I did find it useful.

v1-0012-bufmgr-debug-Add-PrintBuffer-Desc.patch is not used in the patch
series, but I found it quite useful when debugging issues with the patch. A
heck of a lot easier to interpret page flags when they can be printed.

I also think there's some architectural questions that'll influence the number
of patches. E.g. I'm not convinced
v1-0010-heapam-Add-num_pages-to-RelationGetBufferForTuple.patch is quite the
right spot to track which additional pages should be used. It could very well
instead be alongside ->smgr_targblock.  Possibly the best path would instead
be to return the additional pages explicitly to callers of
RelationGetBufferForTuple, but RelationGetBufferForTuple does a bunch of work
around pinning that potentially would need to be repeated in heap_multi_insert().



> > In workloads that extend relations a lot, we end up being extremely contended
> > on the relation extension lock. We've attempted to address that to some degree
> > by using batching, which helps, but only so much.
> 
> Yes, I too have observed this in the past for parallel inserts in CTAS
> work - https://www.postgresql.org/message-id/CALj2ACW9BUoFqWkmTSeHjFD-W7_00s3orqSvtvUk%2BKD2H7ZmRg%40mail.gmail.com.
> Tackling bulk relation extension problems will unblock the parallel
> inserts (in CTAS, COPY) work I believe.

Yea. There's a lot of places the current approach ended up being a bottleneck.


> Firstly, 0001 avoids extra loop over waiters and looks a reasonable
> change, some comments on the patch:

> 1)
> +    int            lwWaiting;        /* 0 if not waiting, 1 if on
> waitlist, 2 if
> +                                 * waiting to be woken */
> Use macros instead of hard-coded values for better readability?
> 
> #define PROC_LW_LOCK_NOT_WAITING 0
> #define PROC_LW_LOCK_ON_WAITLIST 1
> #define PROC_LW_LOCK_WAITING_TO_BE_WOKEN 2

Yea - this was really more of a prototype patch - I noted that we'd want to
use defines for this in
https://www.postgresql.org/message-id/20221027165914.2hofzp4cvutj6gin%40awork3.anarazel.de


> 3)
> +        proclist_delete(&lock->waiters, MyProc->pgprocno, lwWaitLink);
> +        found = true;
> I guess 'found' is a bit meaningless here as we are doing away with
> the proclist_foreach_modify loop. We can directly use
> MyProc->lwWaiting == 1 and remove 'found'.

We can rename it, but I think we still do need it, it's easier to analyze the
logic if the relevant check happens on a value from while we held the wait
list lock. Probably should do the reset inside the locked section as well.


> 4)
>             if (!MyProc->lwWaiting)
>             if (!proc->lwWaiting)
> Can we modify the above conditions in lwlock.c to MyProc->lwWaiting !=
> 1 or PROC_LW_LOCK_ON_WAITLIST or the macro?

I think it's better to check it's 0, rather than just != 1.


> 5) Is there any specific test case that I can see benefit of this
> patch? If yes, can you please share it here?

Yep, see the other thread, there's a pretty easy case there. You can also see
it at extreme client counts with a pgbench -S against a cluster with a smaller
shared_buffers. But the difference is not huge before something like 2048-4096
clients, and then it only occurs occasionally (because you need to end up with
most connections waiting for one of the partitions). So the test case from the
other thread is a lot better.

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.