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: Tue, 9 Feb 2021 18:13:37 +0900
Message-ID: <990e5d4b-075f-101f-aec5-bb44f9b30550@oss.nttdata.com> (raw)
In-Reply-To: <d45e44d655f2ba06d9e722d3dbcfdd79@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>
<8002219d-b999-12fa-2327-49afe40e5fbb@oss.nttdata.com>
<88c451c7-2729-4329-fbae-6ed7664adea1@oss.nttdata.com>
<f9153182845e5584b954ab6f03514d13@oss.nttdata.com>
<ef0dd522-9096-a60f-a0fa-5f0a0fdbafbd@oss.nttdata.com>
<3db333a9-31f7-a225-8038-eeb835ba8f37@oss.nttdata.com>
<b8425207811a6b99d0ecd76844906a8e@oss.nttdata.com>
<f77120a3-4762-bf71-57d5-f0be081715f7@oss.nttdata.com>
<d45e44d655f2ba06d9e722d3dbcfdd79@oss.nttdata.com>
On 2021/02/09 17:48, torikoshia wrote:
> On 2021-02-05 18:49, Fujii Masao wrote:
>> On 2021/02/05 0:03, torikoshia wrote:
>>> On 2021-02-03 11:23, Fujii Masao wrote:
>>>>> 64-bit fetches are not atomic on some platforms. So spinlock is necessary when updating "waitStart" without holding the partition lock? Also GetLockStatusData() needs spinlock when reading "waitStart"?
>>>>
>>>> Also it might be worth thinking to use 64-bit atomic operations like
>>>> pg_atomic_read_u64(), for that.
>>>
>>> Thanks for your suggestion and advice!
>>>
>>> In the attached patch I used pg_atomic_read_u64() and pg_atomic_write_u64().
>>>
>>> waitStart is TimestampTz i.e., int64, but it seems pg_atomic_read_xxx and pg_atomic_write_xxx only supports unsigned int, so I cast the type.
>>>
>>> I may be using these functions not correctly, so if something is wrong, I would appreciate any comments.
>>>
>>>
>>> About the documentation, since your suggestion seems better than v6, I used it as is.
>>
>> Thanks for updating the patch!
>>
>> + if (pg_atomic_read_u64(&MyProc->waitStart) == 0)
>> + pg_atomic_write_u64(&MyProc->waitStart,
>> + pg_atomic_read_u64((pg_atomic_uint64 *) &now));
>>
>> pg_atomic_read_u64() is really necessary? I think that
>> "pg_atomic_write_u64(&MyProc->waitStart, now)" is enough.
>>
>> + deadlockStart = get_timeout_start_time(DEADLOCK_TIMEOUT);
>> + pg_atomic_write_u64(&MyProc->waitStart,
>> + pg_atomic_read_u64((pg_atomic_uint64 *) &deadlockStart));
>>
>> Same as above.
>>
>> + /*
>> + * Record waitStart reusing the deadlock timeout timer.
>> + *
>> + * It would be ideal this can be synchronously done with updating
>> + * lock information. Howerver, since it gives performance impacts
>> + * to hold partitionLock longer time, we do it here asynchronously.
>> + */
>>
>> IMO it's better to comment why we reuse the deadlock timeout timer.
>>
>> proc->waitStatus = waitStatus;
>> + pg_atomic_init_u64(&MyProc->waitStart, 0);
>>
>> pg_atomic_write_u64() should be used instead? Because waitStart can be
>> accessed concurrently there.
>>
>> I updated the patch and addressed the above review comments. Patch attached.
>> Barring any objection, I will commit this version.
>
> Thanks for modifying the patch!
> I agree with your comments.
>
> BTW, I ran pgbench several times before and after applying
> this patch.
>
> The environment is virtual machine(CentOS 8), so this is
> just for reference, but there were no significant difference
> in latency or tps(both are below 1%).
Thanks for the test! I pushed the patch.
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: <990e5d4b-075f-101f-aec5-bb44f9b30550@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