Re: Add “FOR UPDATE NOWAIT” lock details to the log.

On 2025/02/21 10:28, Fujii Masao wrote:
>
>
> On 2025/02/20 15:27, Yuki Seino wrote:
>>
>> On 2025/02/19 1:08, Fujii Masao wrote:
>>> Just an idea, but how about updating ConditionalXactLockTableWait()
>>> to do the followings?
>>> - Use LockAcquireExtended() instead of LockAcquire() to retrieve the
>>> LOCALLOCK value.
>>> - Generate a log message about the lock holders, from the retrieved
>>> the LOCALLOCK value.
>>> - Return the generated log message to the caller
>>> (heap_lock_tuple()), allowing it to log the message.
>> Thank you for your suggestion. I have two issues to discuss:
>>
>>
>> ### 1. Issue with LockAcquireExtended()
>>
>> It appears that in the dontWait case, LockAcquireExtended() is
>> removing the local lock (locallock).
>>
>> ```
>> if (locallock->nLocks == 0)
>>      RemoveLocalLock(locallock);
>> ```
>>
>> Instead, the removal of locallock should be handled by
>> ConditionalXactLockTableWait().
>
> RemoveLocalLock() doesn't seem to remove the necessary LocalLock
> information
> for generating the lock failure log message. Is that correct?
>
> One idea I considered is using the LocalLock returned by
> LockAcquireExtended(),
> but this might complicate the code more than expected. A simpler
> approach could
> be adding a new argument, like logLockFailure, to LockAcquireExtended(),
> allowing it to log the failure message when set to true. This change
> would
> affect LockAcquireExtended() and some ConditionalLockXXX() functions,
> but it doesn't seem too invasive.
>
> As for LockAcquire(), I don't think it needs a new argument. If any
> ConditionalLockXXX() functions call LockAcquire(), we could modify
> them to
> call LockAcquireExtended() instead, enabling logging when needed.
>
>
>> ### 2. Issue with the LOCK TABLE Case
>>
>> For the LOCK TABLE scenario, RangeVarGetRelidExtended() calls
>> ConditionalLockRelationOid(), which seems to require a similar
>> modification.
>
> Yes, if we want to support LOCK TABLE NOWAIT and other NOWAIT cases,
> these additional updates would be required. However, I suggest focusing
> on row-level locks with SELECT ... NOWAIT first, then expanding to
> other cases later.
Thanks for the advice.
I have taken your advice and am working on a patch.

But, I have a problem.
There are 4 locations where LockWaitError is determined.
3 of them could be reproduced, but 1 could not.
I will check more in depth.

(1)
https://github.com/postgres/postgres/blob/master/src/backend/access/heap/heapam.c#L4946
tx1=# begin;
tx1=# select * from pgbench_accounts where aid = '1' for update;
tx2=# select * from pgbench_accounts where aid = '1' for update nowait;

(2)
https://github.com/postgres/postgres/blob/master/src/backend/access/heap/heapam.c#L4905
tx1=# begin;
tx1=# select * from pgbench_accounts where aid = '1' for share;
tx2=# begin;
tx2=# select * from pgbench_accounts where aid = '1' for share;
tx3=# select * from pgbench_accounts where aid = '1' for update nowait;

(3)
https://github.com/postgres/postgres/blob/master/src/backend/access/heap/heapam.c#L5211
tx1=# begin;
tx1=# select * from pgbench_accounts where aid = '1' for update;
tx2=# begin;
tx2=# select * from pgbench_accounts where aid = '1' for update;
tx3=# select * from pgbench_accounts where aid = '1' for update nowait;

(4)
https://github.com/postgres/postgres/blob/master/src/backend/access/heap/heapam_handler.c#L463
...I don't know how to reproduce it.

Regards,