Re: adding wait_start column to pg_locks
От | Fujii Masao |
---|---|
Тема | Re: adding wait_start column to pg_locks |
Дата | |
Msg-id | 990e5d4b-075f-101f-aec5-bb44f9b30550@oss.nttdata.com обсуждение исходный текст |
Ответ на | Re: adding wait_start column to pg_locks (torikoshia <torikoshia@oss.nttdata.com>) |
Ответы |
Re: adding wait_start column to pg_locks
(Fujii Masao <masao.fujii@oss.nttdata.com>)
|
Список | pgsql-hackers |
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 holdingthe 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 unsignedint, 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
В списке pgsql-hackers по дате отправления: