Re: adding wait_start column to pg_locks

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.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION