Horton

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

Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>

View thread View on postgr.es
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
To: Andres Freund <andres@anarazel.de>
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-29T13:03:53Z
Lists: pgsql-hackers 
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?

> 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.

> The fundamental issue, in my opinion, is that we do *way* too much while
> holding the relation extension lock.  We acquire a victim buffer, if that
> buffer is dirty, we potentially flush the WAL, then write out that
> buffer. Then we zero out the buffer contents. Call smgrextend().
>
> Most of that work does not actually need to happen while holding the relation
> extension lock. As far as I can tell, the minimum that needs to be covered by
> the extension lock is the following:
>
> 1) call smgrnblocks()
> 2) insert buffer[s] into the buffer mapping table at the location returned by
>    smgrnblocks
> 3) mark buffer[s] as IO_IN_PROGRESS

Makes sense.

I will try to understand and review each patch separately.

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

2) Missing initialization of lwWaiting to 0 or the macro in twophase.c
and proc.c.
    proc->lwWaiting = false;
    MyProc->lwWaiting = false;

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'.

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?

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

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

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.