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
