Subject: Re: adding wait_start column to pg_locks
To: torikoshia <torikoshia@oss.nttdata.com>
Cc: Ian Lawrence Barwick <barwick@gmail.com>,
 Robert Haas <robertmhaas@gmail.com>, Justin Pryzby <pryzby@telsasoft.com>,
 pgsql-hackers <pgsql-hackers@postgresql.org>
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>
From: Fujii Masao <masao.fujii@oss.nttdata.com>
Message-ID: <ef0dd522-9096-a60f-a0fa-5f0a0fdbafbd@oss.nttdata.com>
Date: Wed, 3 Feb 2021 01:49:46 +0900
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:78.0)
 Gecko/20100101 Thunderbird/78.7.0
MIME-Version: 1.0
In-Reply-To: <f9153182845e5584b954ab6f03514d13@oss.nttdata.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 2021/02/02 22:00, torikoshia wrote:
> On 2021-01-25 23:44, Fujii Masao wrote:
>> Another comment is; Doesn't the change of MyProc->waitStart need the
>> lock table's partition lock? If yes, we can do that by moving
>> LWLockRelease(partitionLock) just after the change of
>> MyProc->waitStart, but which causes the time that lwlock is being held
>> to be long. So maybe we need another way to do that.
> 
> Thanks for your comments!
> 
> It would be ideal for the consistency of the view to record "waitstart" during holding the table partition lock.
> However, as you pointed out, it would give non-negligible performance impacts.
> 
> I may miss something, but as far as I can see, the influence of not holding the lock is that "waitstart" can be NULL even though "granted" is false.
> 
> I think people want to know the start time of the lock when locks are held for a long time.
> In that case, "waitstart" should have already been recorded.

Sounds reasonable.


> If this is true, I think the current implementation may be enough on the condition that users understand it can happen that "waitStart" is NULL and "granted" is false.
> 
> Attached a patch describing this in the doc and comments.
> 
> 
> Any Thoughts?

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"?


+       Lock acquisition wait start time.

Isn't it better to describe this more clearly? What about the following?

     Time when the server process started waiting for this lock,
     or null if the lock is held.

+       Note that updating this field and lock acquisition are not performed
+       synchronously for performance reasons.  Therefore, depending on the
+       timing, it can happen that <structfield>waitstart</structfield> is
+       <literal>NULL</literal> even though
+       <structfield>granted</structfield> is false.

I agree that it's helpful to add the note about that NULL can be returned even when "granted" is false. But IMO we don't need to document why this behavior can happen internally. So what about the following?

     Note that this can be null for a very short period of time after
     the wait started even though <structfield>granted</structfield>
     is <literal>false</literal>.

Since the document for pg_locks uses "null" instead of <literal>NULL</literal> (I'm not sure why, though), I used "null" for the sake of consistency.

Regards,

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