Re: Add “FOR UPDATE NOWAIT” lock details to the log. - Mailing list pgsql-hackers

On 2025/01/17 18:29, Yuki Seino wrote:
> Thank you for your comments.
> 
> 
>> + Currently, only lock failures due to <literal>NOWAIT</literal> are
>> +        supported.  The default is <literal>off</literal>.  Only superusers
>>
>> This GUC also affects SKIP LOCKED, so that should be documented.
> 
> I overlooked it... I have revised the document with SKIP LOCKED.

SELECT FOR UPDATE SKIP LOCKED might skip a large number of rows, leading to
a high volume of log messages from log_lock_failure in your current patch.
Could this be considered unintended behavior? Would it be better to limit
the number of log messages per query?


> I moved the logic to report lock holders and waiters after JoinWaitQueue().

Thanks for updating the patch!


+/*
+ *         CollectLockHoldersAndWaiters

Why was this function moved to lmgr.c? Wouldn't it make more sense to
keep it in proc.c, as the original code was located there?


+ * we loop over the lock's procLocks to gather a list of all
+ * holders and waiters. Thus we will be able to provide more
+ * detailed information for lock debugging purposes.
+ *
+ * lock->procLocks contains all processes which hold or wait for
+ * this lock.

Since this seems more like an internal comment, it would be better
placed directly above the dlist_foreach(proc_iter, &lock->procLocks) loop.
For the function header, consider a description about interface, such as:

--------------
CollectLockHoldersAndWaiters -- gather details about lock holders and waiters

The lock table's partition lock must be held on entry and remains held on exit.

Fill lock_holders_sbuf and lock_waiters_sbuf with the PIDs of processes holding
and waiting for the lock, and set lockHoldersNum to the number of lock holders.
--------------


To ensure correctness, it would be better to assert that the partition lock
is held on entry. For example, you could add the following assertion in
CollectLockHoldersAndWaiters(). If so, the argument "LOCK *lock" should be
changed to "LOCALLOCK *locallock".

--------------
#ifdef USE_ASSERT_CHECKING
    {
        uint32        hashcode = locallock->hashcode;
        LWLock       *partitionLock = LockHashPartitionLock(hashcode);

        Assert(!LWLockHeldByMe(partitionLock));
    }
#endif
--------------


+    dlist_foreach(proc_iter, &lock->procLocks)

lockHoldersNum should be initialized to zero before the loop to handle cases
where the caller forgets to do so.


+/* GUC variables. */
+int            max_locks_per_xact; /* This configuration variable is used to set the lock table size */
+bool        log_lock_failure = false;

IMO the declaration of log_lock_failure would be more intuitive if placed
immediately after log_lock_waits in proc.c.


+                int            lockHoldersNum = 0;
+                initStringInfo(&buf);

Please add a line break before initStringInfo(&buf) for better readability.


+                LWLockAcquire(partitionLock, LW_SHARED);
+                DescribeLockTag(&buf, &locallock->tag.lock);
+                modename = GetLockmodeName(locallock->tag.lock.locktag_lockmethodid,
+                                            lockmode);

The partition lock is unnecessary for DescribeLockTag() and GetLockmodeName().
It should only be acquired right before calling CollectLockHoldersAndWaiters()
and released immediately afterward.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION