Horton

Thread

Commits

  1. Rename log_lock_failure GUC to log_lock_failures for consistency.

  2. Add GUC option to log lock acquisition failures.

  3. Split ProcSleep function into JoinWaitQueue and ProcSleep

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

    Yuki Seino <seinoyu@oss.nttdata.com> — 2024-09-13T11:49:36Z

    Hello,

I would like to add the information of the PID that caused the failure
when acquiring a lock with "FOR UPDATE NOWAIT".

When "FOR UPDATE" is executed and interrupted by lock_timeout,
xid and PID are output in the logs, but in the case of "FOR UPDATE 
NOWAIT",
no information is output, making it impossible to identify the cause of 
the lock failure.
Therefore, I would like to output information in the logs in the same 
way as
when "FOR UPDATE" is executed and interrupted by lock_timeout.

The patch is attached as well.

Regards,
--
Yuki Seino
NTT DATA CORPORATION

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

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2024-10-16T11:03:21Z

    
On 2024/09/13 20:49, Seino Yuki wrote:
> Hello,
> 
> I would like to add the information of the PID that caused the failure
> when acquiring a lock with "FOR UPDATE NOWAIT".
> 
> When "FOR UPDATE" is executed and interrupted by lock_timeout,
> xid and PID are output in the logs,

Could you explain how to reproduce this? In my quick test,
lock waits canceled by lock_timeout didn’t report either xid or PID,
so I might be missing something.


> but in the case of "FOR UPDATE NOWAIT",
> no information is output, making it impossible to identify the cause of the lock failure.
> Therefore, I would like to output information in the logs in the same way as
> when "FOR UPDATE" is executed and interrupted by lock_timeout.

I think NOWAIT is often used for lock waits that can fail frequently.
Logging detailed information for each failed NOWAIT lock wait could
lead to excessive and potentially noisy log messages for some users.

On the other hand, I understand that even with NOWAIT, we might want
to investigate why a lock wait failed. In such cases,
detailed information about the locking process would be useful.

Considering both points, I’m leaning toward adding a new GUC parameter
to control whether detailed logs should be generated for failed
NOWAIT locks (and possibly for those canceled by lock_timeout).
Alternatively, if adding a new GUC is not ideal, we could extend
log_lock_waits to accept a new value like 'error,' which would trigger
detailed logging when a lock wait fails due to NOWAIT, lock_timeout,
or cancellation.


> The patch is attached as well.

I got the following compiler errors;

proc.c:1240:3: error: call to undeclared function 'CollectLockHoldersAndWaiters'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
  1240 |                 CollectLockHoldersAndWaiters(proclock, lock, &lock_holders_sbuf, &lock_waiters_sbuf, &lockHoldersNum);
       |                 ^
proc.c:1549:4: error: call to undeclared function 'CollectLockHoldersAndWaiters'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
  1549 |                         CollectLockHoldersAndWaiters(NULL, lock, &lock_holders_sbuf, &lock_waiters_sbuf, &lockHoldersNum);
       |                         ^
proc.c:1126:8: warning: unused variable 'first_holder' [-Wunused-variable]
  1126 |         bool            first_holder = true,
       |                         ^~~~~~~~~~~~
proc.c:1127:5: warning: unused variable 'first_waiter' [-Wunused-variable]
  1127 |                                 first_waiter = true;
       |                                 ^~~~~~~~~~~~
proc.c:1982:1: error: conflicting types for 'CollectLockHoldersAndWaiters'
  1982 | CollectLockHoldersAndWaiters(PROCLOCK *waitProcLock, LOCK *lock, StringInfo lock_holders_sbuf, StringInfo lock_waiters_sbuf, int *lockHoldersNum)
       | ^
proc.c:1240:3: note: previous implicit declaration is here
  1240 |                 CollectLockHoldersAndWaiters(proclock, lock, &lock_holders_sbuf, &lock_waiters_sbuf, &lockHoldersNum);
       |                 ^
2 warnings and 3 errors generated.

Regards,

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

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

    Yuki Seino <seinoyu@oss.nttdata.com> — 2024-10-17T09:06:26Z

    Thank you for Review.

> Could you explain how to reproduce this? In my quick test,
> lock waits canceled by lock_timeout didn’t report either xid or PID,
> so I might be missing something.
It seems to be outputting on my end, how about you?
===== Console =====
postgres=# SHOW log_lock_waits;
  log_lock_waits
----------------
  on
(1 row)

postgres=# SHOW lock_timeout;
  lock_timeout
--------------
  2s
(1 row)

(tx1) postgres=# BEGIN;
       BEGIN
       postgres=*# SELECT aid FROM pgbench_accounts WHERE aid = 1 FOR 
UPDATE;
        aid
       -----
          1
       (1 row)
(tx2) postgres=# SELECT aid FROM pgbench_accounts WHERE aid = 1 FOR 
UPDATE;
       ERROR:  canceling statement due to lock timeout
       CONTEXT:  while locking tuple (0,1) in relation "pgbench_accounts"

===== Log Output =====
# lock start
2024-10-17 17:49:58.157 JST [1443346] LOG:  process 1443346 still 
waiting for ShareLock on transaction 770 after 1000.096 ms
2024-10-17 17:49:58.157 JST [1443346] DETAIL:  Process holding the lock: 
1443241. Wait queue: 1443346.
2024-10-17 17:49:58.157 JST [1443346] CONTEXT:  while locking tuple 
(0,1) in relation "pgbench_accounts"
2024-10-17 17:49:58.157 JST [1443346] STATEMENT:  SELECT * FROM 
pgbench_accounts WHERE aid = 1 FOR UPDATE;
# lock timeout
2024-10-17 17:49:59.157 JST [1443346] ERROR:  canceling statement due to 
lock timeout
2024-10-17 17:49:59.157 JST [1443346] CONTEXT:  while locking tuple 
(0,1) in relation "pgbench_accounts"
2024-10-17 17:49:59.157 JST [1443346] STATEMENT:  SELECT * FROM 
pgbench_accounts WHERE aid = 1 FOR UPDATE;


> I think NOWAIT is often used for lock waits that can fail frequently.
> Logging detailed information for each failed NOWAIT lock wait could
> lead to excessive and potentially noisy log messages for some users.
> 
> On the other hand, I understand that even with NOWAIT, we might want
> to investigate why a lock wait failed. In such cases,
> detailed information about the locking process would be useful.
> 
> Considering both points, I’m leaning toward adding a new GUC parameter
> to control whether detailed logs should be generated for failed
> NOWAIT locks (and possibly for those canceled by lock_timeout).
> Alternatively, if adding a new GUC is not ideal, we could extend
> log_lock_waits to accept a new value like 'error,' which would trigger
> detailed logging when a lock wait fails due to NOWAIT, lock_timeout,
> or cancellation.
That's a good idea. I'll try that.

> I got the following compiler errors;
thank you. I missed it.

Regards,
--
Yuki Seino
NTT DATA CORPORATION

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

    Yuki Seino <seinoyu@oss.nttdata.com> — 2024-10-17T13:15:51Z

    >> Considering both points, I’m leaning toward adding a new GUC parameter
>> to control whether detailed logs should be generated for failed
>> NOWAIT locks (and possibly for those canceled by lock_timeout).
>> Alternatively, if adding a new GUC is not ideal, we could extend
>> log_lock_waits to accept a new value like 'error,' which would trigger
>> detailed logging when a lock wait fails due to NOWAIT, lock_timeout,
>> or cancellation.
> That's a good idea. I'll try that.

Sorry, I misunderstood.
The pid and xid are output during deadlock_timeout.

> LOG:  process 1443346 still waiting for ShareLock on transaction 770 
> after 1000.096 ms
> DETAIL:  Process holding the lock: 1443241. Wait queue: 1443346.
This log output is not triggered by lock_timeout or cancellation.
Therefore, it is difficult to support lock_timeout and cancellation.

I am thinking of supporting only NOWAIT. What do you think?

Regards,
--
Yuki Seino
NTT DATA CORPORATION

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

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2024-10-17T13:29:10Z

    
On 2024/10/17 22:15, Seino Yuki wrote:
> 
>>> Considering both points, I’m leaning toward adding a new GUC parameter
>>> to control whether detailed logs should be generated for failed
>>> NOWAIT locks (and possibly for those canceled by lock_timeout).
>>> Alternatively, if adding a new GUC is not ideal, we could extend
>>> log_lock_waits to accept a new value like 'error,' which would trigger
>>> detailed logging when a lock wait fails due to NOWAIT, lock_timeout,
>>> or cancellation.
>> That's a good idea. I'll try that.
> 
> Sorry, I misunderstood.
> The pid and xid are output during deadlock_timeout.
> 
>> LOG:  process 1443346 still waiting for ShareLock on transaction 770 after 1000.096 ms
>> DETAIL:  Process holding the lock: 1443241. Wait queue: 1443346.
> This log output is not triggered by lock_timeout or cancellation.

Yes, these log messages with PID or XID are generated by log_lock_waits, not lock_timeout.


> Therefore, it is difficult to support lock_timeout and cancellation.
> 
> I am thinking of supporting only NOWAIT. What do you think?

I'm not sure why it's challenging to provide detailed log messages for lock waits canceled
by lock_timeout or user cancellation, while it's considered feasible for the NOWAIT case.
However, I think it's reasonable to implement this feature step by step. We can start
by adding support for the NOWAIT case and consider extending it to handle lock_timeout and
cancellation scenarios later if possible.

Regards,

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

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

    Yuki Seino <seinoyu@oss.nttdata.com> — 2024-10-18T00:37:49Z

    Currently, we are discussing two improvements:

1. Log output when NOWAIT fails.
2. Adding control via GUC parameters (NOWAIT, lock_timeout, 
cancellation).

> I'm not sure why it's challenging to provide detailed log messages for 
> lock waits canceled
> by lock_timeout or user cancellation, while it's considered feasible 
> for the NOWAIT case.

Does this statement mean that for 2, why can NOWAIT but 
lock_timeout,cancellation cannot?

For item 2, the lock_timeout and cancellation will log outputs after the 
deadlock_timeout(e.g. 1s) has elapsed (by log_lock_waits).
At the time this log is output, it is unclear whether the lock will be 
cancellation or lock_timeout.
This means that the timing at "error is determined" and "output logged" 
do not match.

> However, I think it's reasonable to implement this feature step by 
> step. We can start
> by adding support for the NOWAIT case and consider extending it to 
> handle lock_timeout and
> cancellation scenarios later if possible.

+1.
I will send the version with the GUC parameter added from the previous 
patch.

Regards,
--
Yuki Seino
NTT DATA CORPORATION

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

    Yuki Seino <seinoyu@oss.nttdata.com> — 2024-10-18T01:45:24Z

    > I will send the version with the GUC parameter added from the previous 
> patch.
I made some minor code refactoring.

Regards,
--
Yuki Seino
NTT DATA CORPORATION

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

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2024-10-18T04:58:19Z

    
On 2024/10/18 9:37, Seino Yuki wrote:
> Currently, we are discussing two improvements:
> 
> 1. Log output when NOWAIT fails.
> 2. Adding control via GUC parameters (NOWAIT, lock_timeout, cancellation).
> 
>> I'm not sure why it's challenging to provide detailed log messages for lock waits canceled
>> by lock_timeout or user cancellation, while it's considered feasible for the NOWAIT case.
> 
> Does this statement mean that for 2, why can NOWAIT but lock_timeout,cancellation cannot?
> 
> For item 2, the lock_timeout and cancellation will log outputs after the deadlock_timeout(e.g. 1s) has elapsed (by log_lock_waits).

This log message isn't directly related to lock_timeout or cancellations. It appears
if log_lock_waits is enabled and the lock wait time exceeds deadlock_timeout,
regardless of whether lock_timeout is set or if users cancel the operation.

I understood your original proposal as suggesting detailed log output whenever
a NOWAIT lock attempt fails. However, as I mentioned earlier, NOWAIT failures can
happen frequently, so logging every failure in detail could be too noisy for some users.
That’s why I proposed adding a new GUC parameter (or extending log_lock_waits)
to control whether such detailed logs should be generated for NOWAIT failures.

During our discussion, I also thought it would be useful to provide detailed logs
for lock waits canceled by user actions or lock_timeout. This behavior could be
controlled by a similar GUC parameter. However, this is an additional feature
beyond your original proposal, so I think it's fine to leave it out of the initial patch.


> At the time this log is output, it is unclear whether the lock will be cancellation or lock_timeout.
> This means that the timing at "error is determined" and "output logged" do not match.
> 
>> However, I think it's reasonable to implement this feature step by step. We can start
>> by adding support for the NOWAIT case and consider extending it to handle lock_timeout and
>> cancellation scenarios later if possible.
> 
> +1.
> I will send the version with the GUC parameter added from the previous patch.

The choice between adding a new GUC or extending the existing one (e.g., log_lock_waits)
is debatable, but I prefer the latter. I'm considering extending log_lock_waits
to accept a value like "fail". If set to "on" (the current behavior),
detailed logs are generated when the lock wait time exceeds deadlock_timeout.
If set to "fail", logs are generated whenever a lock wait fails. If both are
specified, logs would be triggered when the wait time exceeds deadlock_timeout or
when a lock wait fails.

Regards,

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

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

    Yuki Seino <seinoyu@oss.nttdata.com> — 2024-10-18T06:00:50Z

    > During our discussion, I also thought it would be useful to provide detailed logs 
> > for lock waits canceled by user actions or lock_timeout. 

Thank you. I understand now.
You want to output the logs based on a different trigger than 
log_lock_waits.

At first, I found that strange, but I see now that there are cases where 
it's necessary to output logs for errors even when log_lock_waits is 
disabled.

I have also understood the proposal for improving the GUC parameters.
I will revise the patch.

Regards,
--
Yuki Seino
NTT DATA CORPORATION

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

    Yuki Seino <seinoyu@oss.nttdata.com> — 2024-10-18T10:07:28Z

    > The choice between adding a new GUC or extending the existing one
> (e.g., log_lock_waits)
> is debatable, but I prefer the latter. I'm considering extending 
> log_lock_waits
> to accept a value like "fail". If set to "on" (the current behavior),
> detailed logs are generated when the lock wait time exceeds 
> deadlock_timeout.
> If set to "fail", logs are generated whenever a lock wait fails. If 
> both are
> specified, logs would be triggered when the wait time exceeds
> deadlock_timeout or
> when a lock wait fails.

Thanks for the idea.
Changed log_lock_waits to an enum type and added fail and all.
"off"  : No log message(default).
"on"   : If over deadlock_timeout(the current behavior).
"fail" : If lock failed.
"all"  : All pettern.

I struggled with the name "on," but decided to leave it by 
compatibility.

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

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2024-12-02T17:13:53Z

    
On 2024/10/18 19:07, Seino Yuki wrote:
>> The choice between adding a new GUC or extending the existing one
>> (e.g., log_lock_waits)
>> is debatable, but I prefer the latter. I'm considering extending log_lock_waits
>> to accept a value like "fail". If set to "on" (the current behavior),
>> detailed logs are generated when the lock wait time exceeds deadlock_timeout.
>> If set to "fail", logs are generated whenever a lock wait fails. If both are
>> specified, logs would be triggered when the wait time exceeds
>> deadlock_timeout or
>> when a lock wait fails.
> 
> Thanks for the idea.
> Changed log_lock_waits to an enum type and added fail and all.
> "off"  : No log message(default).
> "on"   : If over deadlock_timeout(the current behavior).
> "fail" : If lock failed.
> "all"  : All pettern.

I'm still thinking about how we should handle logging for lock failures
caused by the nowait option. Extending the existing log_lock_waits parameter
has the advantage of avoiding a new GUC, but it might make configuration
more complicated. I'm starting to think adding a new GUC might be a better option.

Regarding the patch, when I applied it to HEAD, it failed to compile with
the following errors. Could you update the patch to address this?

proc.c:1538:20: error: use of undeclared identifier 'buf'
  1538 |                         initStringInfo(&buf);
       |                                         ^
proc.c:1539:20: error: use of undeclared identifier 'lock_waiters_sbuf'
  1539 |                         initStringInfo(&lock_waiters_sbuf);
       |                                         ^
proc.c:1540:20: error: use of undeclared identifier 'lock_holders_sbuf'
  1540 |                         initStringInfo(&lock_holders_sbuf);
       |                                         ^
....


Regards,

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

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

    Yuki Seino <seinoyu@oss.nttdata.com> — 2024-12-18T09:17:50Z

    >>> ing a new GUC or extending the existing one
>>> (e.g., log_lock_waits)
>>> is debatable, but I prefer the latter. I'm considering extending 
>>> log_lock_waits
>>> to accept a value like "fail". If set to "on" (the current behavior),
>>> detailed logs are generated when the lock wait time exceeds 
>>> deadlock_timeout.
>>> If set to "fail", logs are generated whenever a lock wait fails. If 
>>> both are
>>> specified, logs would be triggered when the wait time exceeds
>>> deadlock_timeout or
>>> when a lock wait fails.
>>
>> Thanks for the idea.
>> Changed log_lock_waits to an enum type and added fail and all.
>> "off"  : No log message(default).
>> "on"   : If over deadlock_timeout(the current behavior).
>> "fail" : If lock failed.
>> "all"  : All pettern.
>
> I'm still thinking about how we should handle logging for lock failures
> caused by the nowait option. Extending the existing log_lock_waits 
> parameter
> has the advantage of avoiding a new GUC, but it might make configuration
> more complicated. I'm starting to think adding a new GUC might be a 
> better option.

Yes. It’s still simple for now, but reusing an existing GUC could 
complicate future expansions.

I will design a new GUC while ensuring consistency with 'log_lock_waits'.


> Regarding the patch, when I applied it to HEAD, it failed to compile with
> the following errors. Could you update the patch to address this?
>
> proc.c:1538:20: error: use of undeclared identifier 'buf'
>  1538 |                         initStringInfo(&buf);
>       |                                         ^
> proc.c:1539:20: error: use of undeclared identifier 'lock_waiters_sbuf'
>  1539 | initStringInfo(&lock_waiters_sbuf);
>       |                                         ^
> proc.c:1540:20: error: use of undeclared identifier 'lock_holders_sbuf'
>  1540 | initStringInfo(&lock_holders_sbuf);
>       |                                         ^
> ....

Conflicted with another commit [1]. I'll rebase it later.


[1] 
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=3c0fd64fec8ed6fa3987c33f076fcffbc3f268c3

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

    Yuki Seino <seinoyu@oss.nttdata.com> — 2024-12-19T08:21:20Z

    > I will design a new GUC while ensuring consistency with 'log_lock_waits'.
>
>> Regarding the patch, when I applied it to HEAD, it failed to compile 
>> with
>> the following errors. Could you update the patch to address this?
>>
>> proc.c:1538:20: error: use of undeclared identifier 'buf'
>>  1538 |                         initStringInfo(&buf);
>>       |                                         ^
>> proc.c:1539:20: error: use of undeclared identifier 'lock_waiters_sbuf'
>>  1539 | initStringInfo(&lock_waiters_sbuf);
>>       |                                         ^
>> proc.c:1540:20: error: use of undeclared identifier 'lock_holders_sbuf'
>>  1540 | initStringInfo(&lock_holders_sbuf);
>>       |                                         ^
>> ....
>
> Conflicted with another commit [1]. I'll rebase it later.
>
>
> [1] 
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=3c0fd64fec8ed6fa3987c33f076fcffbc3f268c3
Rebased and added new GUC log_lock_failure and minor fixes. Currently 
only NOWAIT errors are supported.
I would like to revisit the possibility of extending this GUC to include
client cancellations and lock timeouts at another opportunity.

-- 
Regards,
Yuki Seino
NTT DATA GROUP CORPORATION

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

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-01-08T05:24:56Z

    
On 2024/12/19 17:21, Yuki Seino wrote:
>> [1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=3c0fd64fec8ed6fa3987c33f076fcffbc3f268c3
> Rebased and added new GUC log_lock_failure and minor fixes. Currently only NOWAIT errors are supported.

Thanks for updating the patch!

+        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.

Regarding the GUC name log_lock_failure, I'm not sure it's the best fit,
but I don't have a better suggestion at the moment. I'll keep considering alternatives.

If CollectLockHoldersAndWaiters() is used only in proc.c,
it should be made a static function.

+/*
+ * 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.

For the source comment of CollectLockHoldersAndWaiters(),
could you follow the style of other functions in proc.c?

Why is the logic to report lock holders and waiters implemented within JoinWaitQueue()?
Wouldn't it be better placed right after JoinWaitQueue() is called in
LockAcquireExtended(), similar to DeadLockReport()? One issue with
the current implementation is that partitionLock is held in exclusive mode
while collecting and reporting lock holders and waiters. If the logic is moved
after JoinWaitQueue() in LockAcquireExtended(), lock holders and waiters could be
processed after releasing partitionLock. Note, however, that partitionLock might
still need to be taken again in shared mode during the collection.

Regards,

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

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

    Yuki Seino <seinoyu@oss.nttdata.com> — 2025-01-17T09:29:50Z

    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.

> If CollectLockHoldersAndWaiters() is used only in proc.c,
> it should be made a static function.
>
> +/*
> + * 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.
>
> For the source comment of CollectLockHoldersAndWaiters(),
> could you follow the style of other functions in proc.c?
>
> Why is the logic to report lock holders and waiters implemented within 
> JoinWaitQueue()?
> Wouldn't it be better placed right after JoinWaitQueue() is called in
> LockAcquireExtended(), similar to DeadLockReport()? One issue with
> the current implementation is that partitionLock is held in exclusive 
> mode
> while collecting and reporting lock holders and waiters. If the logic 
> is moved
> after JoinWaitQueue() in LockAcquireExtended(), lock holders and 
> waiters could be
> processed after releasing partitionLock. Note, however, that 
> partitionLock might
> still need to be taken again in shared mode during the collection.

Indeed.

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


Regards,

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

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-01-17T17:58:19Z

    
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

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

    Yuki Seino <seinoyu@oss.nttdata.com> — 2025-02-03T12:30:42Z

    Thank you for your comment. Sorry for being late.


> 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?
It is necessary to suppress the generation of a large amount of logs due 
to SKIP LOCKED.
But I think that when using SKIP LOCKED, the locks are often 
intentionally bypassed.
It seems unnatural to log only the first write or to set a specific 
number of samples.

What do you think if we simply don't log anything for SKIP LOCKED?


Regards,

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

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-02-12T11:32:11Z

    
On 2025/02/03 21:30, Yuki Seino wrote:
> Thank you for your comment. Sorry for being late.
> 
> 
>> 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?
> It is necessary to suppress the generation of a large amount of logs due to SKIP LOCKED.
> But I think that when using SKIP LOCKED, the locks are often intentionally bypassed.
> It seems unnatural to log only the first write or to set a specific number of samples.

I don't think so. Some users of SKIP LOCKED may want to understand why locks
were skipped and identify the blockers. Even partial information could still
be valuable to them, I think.

> What do you think if we simply don't log anything for SKIP LOCKED?

Implementing both NOWAIT and SKIP LOCKED could take time and make the patch
more complex. I'm fine with focusing on the NOWAIT case first as an initial patch.

Regards,

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

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

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2025-02-12T17:31:53Z

    On Wed, 12 Feb 2025 at 12:32, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> > What do you think if we simply don't log anything for SKIP LOCKED?
>
> Implementing both NOWAIT and SKIP LOCKED could take time and make the patch
> more complex. I'm fine with focusing on the NOWAIT case first as an initial patch.

I think that makes sense. It's a fairly common pattern to use SKIP
LOCKED to implement a concurrent job queue. Having such a usecase
suddenly create lots of logs seems undesirable, especially since it
created no logs at all before. Since NOWAIT already results in an
error (and thus a log), having it add some additional info seems
totally reasonable.

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

    Yuki Seino <seinoyu@oss.nttdata.com> — 2025-02-18T09:33:11Z

    On 2025/02/13 2:31, Jelte Fennema-Nio wrote:
> On Wed, 12 Feb 2025 at 12:32, Fujii Masao<masao.fujii@oss.nttdata.com> wrote:
>>> What do you think if we simply don't log anything for SKIP LOCKED?
>> Implementing both NOWAIT and SKIP LOCKED could take time and make the patch
>> more complex. I'm fine with focusing on the NOWAIT case first as an initial patch.
> I think that makes sense. It's a fairly common pattern to use SKIP
> LOCKED to implement a concurrent job queue. Having such a usecase
> suddenly create lots of logs seems undesirable, especially since it
> created no logs at all before. Since NOWAIT already results in an
> error (and thus a log), having it add some additional info seems
> totally reasonable.

Thank you for the advice. For now, my goal is to output only NOWAIT. 
Since lock.c cannot reference LockWaitPolicy, I believe we need to 
extend the IF conditions in LockAcquire, LockAcquireExtended, and their 
higher-level functions. This could be a pretty significant modification. 
I’ll think about whether there’s a better approach. I welcome any good 
ideas from everyone too. As an aside, I also noticed that 
dontWait(=true) is routed not only from NOWAIT and SKIP LOCKED but also 
from vacuum and other parts. do_autovacuum(autovacuum.c) -> 
ConditionalLockDatabaseObject -> LockAcquireExtended Regards,

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

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-02-18T16:08:45Z

    
On 2025/02/18 18:33, Yuki Seino wrote:
> 
> On 2025/02/13 2:31, Jelte Fennema-Nio wrote:
>> On Wed, 12 Feb 2025 at 12:32, Fujii Masao<masao.fujii@oss.nttdata.com> wrote:
>>>> What do you think if we simply don't log anything for SKIP LOCKED?
>>> Implementing both NOWAIT and SKIP LOCKED could take time and make the patch
>>> more complex. I'm fine with focusing on the NOWAIT case first as an initial patch.
>> I think that makes sense. It's a fairly common pattern to use SKIP
>> LOCKED to implement a concurrent job queue. Having such a usecase
>> suddenly create lots of logs seems undesirable, especially since it
>> created no logs at all before. Since NOWAIT already results in an
>> error (and thus a log), having it add some additional info seems
>> totally reasonable.
> 
> Thank you for the advice. For now, my goal is to output only NOWAIT. Since lock.c cannot reference LockWaitPolicy, I believe we need to extend the IF conditions in LockAcquire, LockAcquireExtended, and their higher-level functions. This could be a pretty significant modification. I’ll think about whether there’s a better approach. I welcome any good ideas from everyone too. 
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.

Regards,

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

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

    Yuki Seino <seinoyu@oss.nttdata.com> — 2025-02-20T06:27:19Z

    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().


### 2. Issue with the LOCK TABLE Case

For the LOCK TABLE scenario, RangeVarGetRelidExtended() calls 
ConditionalLockRelationOid(), which seems to require a similar modification.


```
# tx1
BEGIN; LOCK TABLE pgbench_accounts;

# tx2
BEGIN; LOCK TABLE pgbench_accounts NOWAIT;
-- breakpoint -> LockAcquireExtended

# parts of CALLSTACK
LockAcquireExtended()
ConditionalLockRelationOid()
RangeVarGetRelidExtended()
LockTableCommand()
standard_ProcessUtility()
ProcessUtility()
```

I don't see a problem with it, though,
Do you think these are problems?

Regards,

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

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-02-21T01:28:31Z

    
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.

Regards,

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

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

    Yuki Seino <seinoyu@oss.nttdata.com> — 2025-02-27T06:44:18Z

    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,

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

    Yuki Seino <seinoyu@oss.nttdata.com> — 2025-02-28T12:08:01Z

    On 2025/02/27 15:44, Yuki Seino wrote:
> (4) 
> https://github.com/postgres/postgres/blob/master/src/backend/access/heap/heapam_handler.c#L463 
>
> ...I don't know how to reproduce it.
I have confirmed that (4) can be reproduced using the following procedure.

(4) 
https://github.com/postgres/postgres/blob/master/src/backend/access/heap/heapam_handler.c#L463
tx1=# SELECT pg_advisory_lock(0);
tx2=# SELECT * FROM pgbench_accounts WHERE pg_advisory_lock(0) IS NOT 
NULL FOR UPDATE NOWAIT;
tx1=# UPDATE pgbench_accounts SET aid = aid WHERE aid = '1';
tx1=# BEGIN;
tx1=# UPDATE pgbench_accounts SET aid = aid WHERE aid = '1';
tx1=# SELECT pg_advisory_unlock(0);

Send the modified patch.

Regard,

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

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-02-28T12:51:10Z

    
On 2025/02/28 21:08, Yuki Seino wrote:
> 
> On 2025/02/27 15:44, Yuki Seino wrote:
>> (4) https://github.com/postgres/postgres/blob/master/src/backend/access/heap/heapam_handler.c#L463
>> ...I don't know how to reproduce it.
> I have confirmed that (4) can be reproduced using the following procedure.
> 
> (4) https://github.com/postgres/postgres/blob/master/src/backend/access/heap/heapam_handler.c#L463
> tx1=# SELECT pg_advisory_lock(0);
> tx2=# SELECT * FROM pgbench_accounts WHERE pg_advisory_lock(0) IS NOT NULL FOR UPDATE NOWAIT;
> tx1=# UPDATE pgbench_accounts SET aid = aid WHERE aid = '1';
> tx1=# BEGIN;
> tx1=# UPDATE pgbench_accounts SET aid = aid WHERE aid = '1';
> tx1=# SELECT pg_advisory_unlock(0);
> 
> Send the modified patch.

Thanks for updating the patch!

I encountered a compilation error with the patch. You can also see the error in the patch tester.
https://cirrus-ci.com/task/5070779370438656

Regards,

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

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

    Yuki Seino <seinoyu@oss.nttdata.com> — 2025-03-10T13:11:17Z

    > I encountered a compilation error with the patch. You can also see the 
> error in the patch tester.
> https://cirrus-ci.com/task/5070779370438656

Sorry, removed some errors and made some fixes.

Regard,

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

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-03-10T18:07:32Z

    
On 2025/03/10 22:11, Yuki Seino wrote:
>> I encountered a compilation error with the patch. You can also see the error in the patch tester.
>> https://cirrus-ci.com/task/5070779370438656
> 
> Sorry, removed some errors and made some fixes.

Thanks for updating the patch!

You modified heap_lock_tuple() to log lock acquisition failures
using the lock holders and waiters lists returned by LockAcquireExtended().
However, this results in almost identical logging code in both heapam.c and
heapam_handler.c. This approach feels a bit complicated, and if we extend
this feature to other lock failure cases, we'd have to duplicate
the same logging logic elsewhere — which isn't ideal.

Instead, wouldn't it be simpler to update LockAcquireExtended() to
take a new argument, like logLockFailure, to control whether
a lock failure should be logged directly? I’ve adjusted the patch
accordingly and attached it. Please let me know what you think!

Regards,

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

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

    Yuki Seino <seinoyu@oss.nttdata.com> — 2025-03-11T07:50:06Z

    >
> Instead, wouldn't it be simpler to update LockAcquireExtended() to
> take a new argument, like logLockFailure, to control whether
> a lock failure should be logged directly? I’ve adjusted the patch
> accordingly and attached it. Please let me know what you think!
>
> Regards,
Thank you!

It's very simple and nice.
It seems like it can also handle other lock failure cases by extending 
logLockFailure.

I agree with this propose.


Regards,

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

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-03-11T13:24:25Z

    
On 2025/03/11 16:50, Yuki Seino wrote:
>>
>> Instead, wouldn't it be simpler to update LockAcquireExtended() to
>> take a new argument, like logLockFailure, to control whether
>> a lock failure should be logged directly? I’ve adjusted the patch
>> accordingly and attached it. Please let me know what you think!
>>
>> Regards,
> Thank you!
> 
> It's very simple and nice.
> It seems like it can also handle other lock failure cases by extending logLockFailure.
> > I agree with this propose.

Thanks for reviewing the patch!

I've made some minor cosmetic adjustments. The updated patch is attached.

Unless there are any objections, I'll proceed with committing it.

Regards,

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

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

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-03-14T15:07:25Z

    
On 2025/03/11 22:24, Fujii Masao wrote:
> 
> 
> On 2025/03/11 16:50, Yuki Seino wrote:
>>>
>>> Instead, wouldn't it be simpler to update LockAcquireExtended() to
>>> take a new argument, like logLockFailure, to control whether
>>> a lock failure should be logged directly? I’ve adjusted the patch
>>> accordingly and attached it. Please let me know what you think!
>>>
>>> Regards,
>> Thank you!
>>
>> It's very simple and nice.
>> It seems like it can also handle other lock failure cases by extending logLockFailure.
>> > I agree with this propose.
> 
> Thanks for reviewing the patch!
> 
> I've made some minor cosmetic adjustments. The updated patch is attached.
> 
> Unless there are any objections, I'll proceed with committing it.

Pushed the patch. Thanks!

Using the newly introduced mechanism, we can now easily extend
the log_lock_failure GUC to support additional NOWAIT lock failures,
such as LOCK TABLE ... NOWAIT, ALTER TABLE ... NOWAIT,
ALTER MATERIALIZED VIEW ... NOWAIT, and ALTER INDEX ... NOWAIT.

I've attached a patch implementing this.

Regards,

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

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

    Yuki Seino <seinoyu@oss.nttdata.com> — 2025-03-21T06:21:57Z

    > Pushed the patch. Thanks!

Thank you. I'm very happy !!


> Using the newly introduced mechanism, we can now easily extend
> the log_lock_failure GUC to support additional NOWAIT lock failures,
> such as LOCK TABLE ... NOWAIT, ALTER TABLE ... NOWAIT,
> ALTER MATERIALIZED VIEW ... NOWAIT, and ALTER INDEX ... NOWAIT.
>
> I've attached a patch implementing this.
That's a very good suggestion.

There is just one thing that bothers me.
ConditionalLockRelationOid() seems to be used by autovacuum as well.
Wouldn't this information be noise to the user?

Regards,

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

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-03-23T14:04:29Z

    
On 2025/03/21 15:21, Yuki Seino wrote:
>> Pushed the patch. Thanks!
> 
> Thank you. I'm very happy !!
> 
> 
>> Using the newly introduced mechanism, we can now easily extend
>> the log_lock_failure GUC to support additional NOWAIT lock failures,
>> such as LOCK TABLE ... NOWAIT, ALTER TABLE ... NOWAIT,
>> ALTER MATERIALIZED VIEW ... NOWAIT, and ALTER INDEX ... NOWAIT.
>>
>> I've attached a patch implementing this.
> That's a very good suggestion.
> 
> There is just one thing that bothers me.
> ConditionalLockRelationOid() seems to be used by autovacuum as well.
> Wouldn't this information be noise to the user?

Yes, logging every autovacuum lock failure would be too noisy.
I was thinking that log_lock_failure should apply only to LOCK TABLE ... NOWAIT,
but per the current code, restricting it that way seems difficult.

Anyway, expanding log_lock_failure further requires more discussion and design work,
which isn't feasible within this CommitFest. So, I'm withdrawing the patch for now.

Regards,

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

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

    Peter Eisentraut <peter@eisentraut.org> — 2025-05-30T10:20:00Z

    On 14.03.25 16:07, Fujii Masao wrote:
>>>> Instead, wouldn't it be simpler to update LockAcquireExtended() to
>>>> take a new argument, like logLockFailure, to control whether
>>>> a lock failure should be logged directly? I’ve adjusted the patch
>>>> accordingly and attached it. Please let me know what you think!
>>>>
>>>> Regards,
>>> Thank you!
>>>
>>> It's very simple and nice.
>>> It seems like it can also handle other lock failure cases by 
>>> extending logLockFailure.
>>> > I agree with this propose.
>>
>> Thanks for reviewing the patch!
>>
>> I've made some minor cosmetic adjustments. The updated patch is attached.
>>
>> Unless there are any objections, I'll proceed with committing it.
> 
> Pushed the patch. Thanks!

This patch added a setting "log_lock_failure", but the existing similar 
setting "log_lock_waits" has a plural.  Is there a reason for this 
difference?  Otherwise, maybe "log_lock_failures" would be better.

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

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-05-30T15:28:49Z

    
On 2025/05/30 19:20, Peter Eisentraut wrote:
> On 14.03.25 16:07, Fujii Masao wrote:
>>>>> Instead, wouldn't it be simpler to update LockAcquireExtended() to
>>>>> take a new argument, like logLockFailure, to control whether
>>>>> a lock failure should be logged directly? I’ve adjusted the patch
>>>>> accordingly and attached it. Please let me know what you think!
>>>>>
>>>>> Regards,
>>>> Thank you!
>>>>
>>>> It's very simple and nice.
>>>> It seems like it can also handle other lock failure cases by extending logLockFailure.
>>>> > I agree with this propose.
>>>
>>> Thanks for reviewing the patch!
>>>
>>> I've made some minor cosmetic adjustments. The updated patch is attached.
>>>
>>> Unless there are any objections, I'll proceed with committing it.
>>
>> Pushed the patch. Thanks!
> 
> This patch added a setting "log_lock_failure", but the existing similar setting "log_lock_waits" has a plural.  Is there a reason for this difference?

No, Seino-san and I went with log_lock_failure at the time because
we didn't have a better name suggestion, and we figured we could
revisit the GUC name later if needed. so thanks for bringing it up again!

>  Otherwise, maybe "log_lock_failures" would be better.

Yes, this seems better for consistency with log_lock_waits.

Regards,

-- 
Fujii Masao
NTT DATA Japan Corporation

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

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-05-30T16:12:46Z

    
On 2025/05/31 0:28, Fujii Masao wrote:
>>   Otherwise, maybe "log_lock_failures" would be better.
> 
> Yes, this seems better for consistency with log_lock_waits.

Patch attached.

Regards,

-- 
Fujii Masao
NTT DATA Japan Corporation

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

    Peter Eisentraut <peter@eisentraut.org> — 2025-06-02T05:42:07Z

    On 30.05.25 18:12, Fujii Masao wrote:
> On 2025/05/31 0:28, Fujii Masao wrote:
>>>   Otherwise, maybe "log_lock_failures" would be better.
>>
>> Yes, this seems better for consistency with log_lock_waits.
> 
> Patch attached.

This looks good to me.

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

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-06-03T01:05:55Z

    
On 2025/06/02 14:42, Peter Eisentraut wrote:
> On 30.05.25 18:12, Fujii Masao wrote:
>> On 2025/05/31 0:28, Fujii Masao wrote:
>>>>   Otherwise, maybe "log_lock_failures" would be better.
>>>
>>> Yes, this seems better for consistency with log_lock_waits.
>>
>> Patch attached.
> 
> This looks good to me.

I've pushed the patch. Thanks!

Regards,

-- 
Fujii Masao
NTT DATA Japan Corporation

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

    cca5507 <cca5507@qq.com> — 2026-05-14T05:27:58Z

    Hi,

> Yes, logging every autovacuum lock failure would be too noisy.
> I was thinking that log_lock_failure should apply only to LOCK TABLE ... NOWAIT,
> but per the current code, restricting it that way seems difficult.

I wrote a simple patch to support LOCK NOWAIT only.

Looking forward to your comments!

--
Regards,
ChangAo Chen

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

    Fujii Masao <masao.fujii@gmail.com> — 2026-06-12T07:27:56Z

    On Thu, May 14, 2026 at 2:28 PM cca5507 <cca5507@qq.com> wrote:
>
> Hi,
>
> > Yes, logging every autovacuum lock failure would be too noisy.
> > I was thinking that log_lock_failure should apply only to LOCK TABLE ... NOWAIT,
> > but per the current code, restricting it that way seems difficult.
>
> I wrote a simple patch to support LOCK NOWAIT only.
>
> Looking forward to your comments!

Thanks for the patch! Could you add it to the next CommitFest so we
don't forget about it?

  RVR_MISSING_OK = 1 << 0, /* don't error if relation doesn't exist */
  RVR_NOWAIT = 1 << 1, /* error if relation cannot be locked */
  RVR_SKIP_LOCKED = 1 << 2, /* skip if relation cannot be locked */
+ RVR_LOG_LOCK_FAILURE = 1 << 3, /* log failure if relation cannot be locked */
 } RVROption;

I understand this approach. But RVROption currently controls
lookup/locking behavior such as MISSING_OK, NOWAIT, and SKIP_LOCKED,
while RVR_LOG_LOCK_FAILURE is different in nature because it controls
logging side effects rather than lookup/lock semantics themselves.

Because of that, it feels a bit out of place in this enum and makes
the API boundary less clear. So I'd prefer to avoid adding
RVR_LOG_LOCK_FAILURE to RVROption.

Instead, how about a simpler approach: update
ConditionalLockRelationOid() to pass log_lock_failures to
LockAcquireExtended()? That would allow not only LOCK TABLE NOWAIT,
but also VACUUM SKIP LOCKED, ALTER ALL IN TABLESPACE NOWAIT, and
REPACK to follow log_lock_failures.

Of course, that would also cause every autovacuum relation lock failure to be
logged, which would likely be too noisy. To avoid that, how about
forcibly disabling log_lock_failures in autovacuum workers? We already
override some parameters there, so perhaps we could add something like
this as well:

    /*
     * Force log_lock_failures OFF in autovacuum workers to avoid verbose
     * logging when maintenance skips contended relations.
    */
    SetConfigOption("log_lock_failures", "false", PGC_SUSET, PGC_S_OVERRIDE);

I think this approach is simpler. Thoughts?

Regards,

-- 
Fujii Masao

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

    cca5507 <cca5507@qq.com> — 2026-06-13T09:59:01Z

    > Thanks for the patch! Could you add it to the next CommitFest so we
> don't forget about it?
> 
>   RVR_MISSING_OK = 1 << 0, /* don't error if relation doesn't exist */
>   RVR_NOWAIT = 1 << 1, /* error if relation cannot be locked */
>   RVR_SKIP_LOCKED = 1 << 2, /* skip if relation cannot be locked */
> + RVR_LOG_LOCK_FAILURE = 1 << 3, /* log failure if relation cannot be locked */
>  } RVROption;
> 
> I understand this approach. But RVROption currently controls
> lookup/locking behavior such as MISSING_OK, NOWAIT, and SKIP_LOCKED,
> while RVR_LOG_LOCK_FAILURE is different in nature because it controls
> logging side effects rather than lookup/lock semantics themselves.
> 
> Because of that, it feels a bit out of place in this enum and makes
> the API boundary less clear. So I'd prefer to avoid adding
> RVR_LOG_LOCK_FAILURE to RVROption.
> 
> Instead, how about a simpler approach: update
> ConditionalLockRelationOid() to pass log_lock_failures to
> LockAcquireExtended()? That would allow not only LOCK TABLE NOWAIT,
> but also VACUUM SKIP LOCKED, ALTER ALL IN TABLESPACE NOWAIT, and
> REPACK to follow log_lock_failures.
> 
> Of course, that would also cause every autovacuum relation lock failure to be
> logged, which would likely be too noisy. To avoid that, how about
> forcibly disabling log_lock_failures in autovacuum workers? We already
> override some parameters there, so perhaps we could add something like
> this as well:
> 
>     /*
>      * Force log_lock_failures OFF in autovacuum workers to avoid verbose
>      * logging when maintenance skips contended relations.
>     */
>     SetConfigOption("log_lock_failures", "false", PGC_SUSET, PGC_S_OVERRIDE);
> 
> I think this approach is simpler. Thoughts?

Yeah, it's simpler, but it might still be too noisy if user do a whole-database
REPACK/VACUUM SKIP LOCKED. Do we need to care about this? If not, I think
your approach is in the right way.

My thought is to support LOCK TABLE NOWAIT only (other NOWAIT commands
can also be supported easily if we need). I agree that RVR_LOG_LOCK_FAILURE is
different from others, so I remove it in the v2 patch. Instead, only log lock failure
when RVR_NOWAIT is set. This avoids noisy logs because we will error out if we
cannot get the lock. Currently only LOCK TABLE NOWAIT uses the RVR_NOWAIT
flag.

I created a CF entry for this:

https://commitfest.postgresql.org/patch/6883/

--
Regards,
ChangAo Chen