From: Fujii Masao <masao.fujii@oss.nttdata.com>
To: torikoshia <torikoshia@oss.nttdata.com>
Cc: Ian Lawrence Barwick <barwick@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Justin Pryzby <pryzby@telsasoft.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: adding wait_start column to pg_locks
Date: Fri, 22 Jan 2021 18:11:54 +0900
Message-ID: <8002219d-b999-12fa-2327-49afe40e5fbb@oss.nttdata.com> (raw)
In-Reply-To: <a5bfd1c2f2fb21eb7ea5ddec2e094430@oss.nttdata.com>
References: <a96013dc51cdc56b2a2b84fa8a16a993@oss.nttdata.com>
<20210101214930.GH25152@telsasoft.com>
<9e958478d8bcea814e7ce16511a18912@oss.nttdata.com>
<CAB8KJ=hN=sz2+u3cQ+5jhZT9+TP-N7OCBLTp68L4fkVoVTqWjw@mail.gmail.com>
<CA+TgmoYowQhMhT74AsxDib6e3LFPGvXVxVOUdP0-hMNY9c1wgw@mail.gmail.com>
<CAB8KJ=idS0m_+65d2ujjjZj5DSwpYYV3XoHMwr-UZ-OFs3Mb4Q@mail.gmail.com>
<23d39ee9c31643fad8f9ba9c5cf3aaf4@oss.nttdata.com>
<0fd375a53e306566cea7f451cc8cfcde@oss.nttdata.com>
<7b1bb07a-73ec-1fcc-2f65-41101adf0080@oss.nttdata.com>
<a5bfd1c2f2fb21eb7ea5ddec2e094430@oss.nttdata.com>
On 2021/01/22 14:37, torikoshia wrote:
> 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.
This change seems to cause "waitstart" to be reset every time
ResolveRecoveryConflictWithLock() is called in the do-while loop.
I guess this is not acceptable. Right?
To avoid that issue, IMO the following change is better. Thought?
- else if (log_recovery_conflict_waits)
+ else
{
+ TimestampTz now = GetCurrentTimestamp();
+
+ MyProc->waitStart = now;
+
/*
* Set the wait start timestamp if logging is enabled and in hot
* standby.
*/
- standbyWaitStart = GetCurrentTimestamp();
+ if (log_recovery_conflict_waits)
+ standbyWaitStart = now
}
This change causes the startup process to call GetCurrentTimestamp()
additionally even when log_recovery_conflict_waits is disabled. Which
might decrease the performance of the startup process, but that performance
degradation can happen only when the startup process waits in
ACCESS EXCLUSIVE lock. So if this my understanding right, IMO it's almost
harmless to call GetCurrentTimestamp() additionally in that case. Thought?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: masao.fujii@oss.nttdata.com, torikoshia@oss.nttdata.com, barwick@gmail.com, robertmhaas@gmail.com, pryzby@telsasoft.com
Subject: Re: adding wait_start column to pg_locks
In-Reply-To: <8002219d-b999-12fa-2327-49afe40e5fbb@oss.nttdata.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox