From: torikoshia <torikoshia@oss.nttdata.com>
To: Fujii Masao <masao.fujii@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, 09 Feb 2021 17:48:55 +0900
Message-ID: <d45e44d655f2ba06d9e722d3dbcfdd79@oss.nttdata.com> (raw)
In-Reply-To: <f77120a3-4762-bf71-57d5-f0be081715f7@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>
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%).
Regards,
--
Atsushi Torikoshi
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: torikoshia@oss.nttdata.com, masao.fujii@oss.nttdata.com, barwick@gmail.com, robertmhaas@gmail.com, pryzby@telsasoft.com
Subject: Re: adding wait_start column to pg_locks
In-Reply-To: <d45e44d655f2ba06d9e722d3dbcfdd79@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