Re: adding wait_start column to pg_locks

On 2021-01-21 12:48, Fujii Masao wrote:

> Thanks for updating the patch! I think that this is really useful 
> feature!!

Thanks for reviewing!

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

Agreed.

I also changed the variable name "wait_start" in struct PGPROC and
LockInstanceData to "waitStart" for the same reason.


> 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?

Thanks, this is because I set 'waitstart' in the following
condition.

   ---src/backend/storage/lmgr/proc.c
   > 1250         if (!InHotStandby)

As far as considering this, I guess startup process would
be the only case.

I also think that in case of startup process, it seems possible
to set 'waitstart' in ResolveRecoveryConflictWithLock(), so I
did it in the attached patch.


Any thoughts?


Regards,

--
Atsushi Torikoshi