On Sat, Oct 29, 2022 at 8:24 AM Andres Freund <and...@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
