Re: adding wait_start column to pg_locks

On 2021/01/18 12:00, torikoshia wrote:
> On 2021-01-15 15:23, torikoshia wrote:
>> Thanks for your reviewing and comments!
>>
>> On 2021-01-14 12:39, Ian Lawrence Barwick wrote:
>>> Looking at the code, this happens as the wait start time is being recorded in
>>> the lock record itself, so always contains the value reported by the latest lock
>>> acquisition attempt.
>>
>> I think you are right and wait_start should not be recorded
>> in the LOCK.
>>
>>
>> On 2021-01-15 11:48, Ian Lawrence Barwick wrote:
>>> 2021年1月15日(金) 3:45 Robert Haas <robertmhaas(at)gmail(dot)com>:
>>>
>>>> On Wed, Jan 13, 2021 at 10:40 PM Ian Lawrence Barwick
>>>> <barwick(at)gmail(dot)com> wrote:
>>>>> It looks like the logical place to store the value is in the
>>>> PROCLOCK
>>>>> structure; ...
>>>>
>>>> That seems surprising, because there's one PROCLOCK for every
>>>> combination of a process and a lock. But, a process can't be waiting
>>>> for more than one lock at the same time, because once it starts
>>>> waiting to acquire the first one, it can't do anything else, and
>>>> thus
>>>> can't begin waiting for a second one. So I would have thought that
>>>> this would be recorded in the PROC.
>>>
>>> Umm, I think we're at cross-purposes here. The suggestion is to note
>>> the time when the process started waiting for the lock in the
>>> process's
>>> PROCLOCK, rather than in the lock itself (which in the original
>>> version
>>> of the patch resulted in all processes with an interest in the lock
>>> appearing
>>> to have been waiting to acquire it since the time a lock acquisition
>>> was most recently attempted).
>>
>> AFAIU, it seems possible to record wait_start in the PROCLOCK but
>> redundant since each process can wait at most one lock.
>>
>> To confirm my understanding, I'm going to make another patch that
>> records wait_start in the PGPROC.
>
> Attached a patch.
>
> I noticed previous patches left the wait_start untouched even after
> it acquired lock.
> The patch also fixed it.
>
> Any thoughts?

Thanks for updating the patch! I think that this is really useful feature!!
I have two minor comments.

+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>wait_start</structfield> <type>timestamptz</type>

The column name "wait_start" should be "waitstart" for the sake of consistency
with other column names in pg_locks? pg_locks seems to avoid including
an underscore in column names, so "locktype" is used instead of "lock_type",
"virtualtransaction" is used instead of "virtual_transaction", etc.

+ Lock acquisition wait start time. <literal>NULL</literal> if
+ lock acquired.

There seems the case where the wait start time is NULL even when "grant"
is false. It's better to add note about that case into the docs? For example,
I found that the wait start time is NULL while the startup process is waiting
for the lock. Is this only that case?

Regards,

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