Re: [PATCH] LockAcquireExtended improvement

On Thu, Feb 1, 2024 at 2:16 AM Jingxian Li <aqktjcm(at)qq(dot)com> wrote:
> According to what you said, I resubmitted a patch which splits the ProcSleep
> logic into two parts, the former is responsible for inserting self to
> WaitQueue,
> the latter is responsible for deadlock detection and processing, and the
> former part is directly called by LockAcquireExtended before nowait fails.
> In this way the nowait case can also benefit from adjusting the insertion
> order of WaitQueue.

I don't have time for a full review of this patch right now
unfortunately, but just looking at it quickly:

- It will be helpful if you write a clear commit message. If it gets
committed, there is a high chance the committer will rewrite your
message, but in the meantime it will help understanding.

- The comment for InsertSelfIntoWaitQueue needs improvement. It is
only one line. And it says "Insert self into queue if dontWait is
false" but then someone will wonder why the function would ever be
called with dontWait = true.

- Between the comments and the commit message, the division of
responsibilities between InsertSelfIntoWaitQueue and ProcSleep needs
to be clearly explained. Right now I don't think it is.

--
Robert Haas
EDB: http://www.enterprisedb.com