Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1l4373-0005Ea-Vz for pgsql-hackers@arkaria.postgresql.org; Mon, 25 Jan 2021 14:45:05 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1l4371-00019C-H9 for pgsql-hackers@arkaria.postgresql.org; Mon, 25 Jan 2021 14:45:03 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1l4370-00018m-Oi for pgsql-hackers@lists.postgresql.org; Mon, 25 Jan 2021 14:45:02 +0000 Received: from oss.nttdata.com ([49.212.34.109]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1l436x-0003QG-Qu for pgsql-hackers@postgresql.org; Mon, 25 Jan 2021 14:45:01 +0000 Received: from hnk.local (p2112118-ipbf2507funabasi.chiba.ocn.ne.jp [123.216.63.118]) by oss.nttdata.com (Postfix) with ESMTPSA id 1AB3562842; Mon, 25 Jan 2021 23:44:57 +0900 (JST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.102.3 at oss.nttdata.com Subject: Re: adding wait_start column to pg_locks From: Fujii Masao To: torikoshia Cc: Ian Lawrence Barwick , Robert Haas , Justin Pryzby , pgsql-hackers References: <20210101214930.GH25152@telsasoft.com> <9e958478d8bcea814e7ce16511a18912@oss.nttdata.com>

<23d39ee9c31643fad8f9ba9c5cf3aaf4@oss.nttdata.com> <0fd375a53e306566cea7f451cc8cfcde@oss.nttdata.com> <7b1bb07a-73ec-1fcc-2f65-41101adf0080@oss.nttdata.com> <8002219d-b999-12fa-2327-49afe40e5fbb@oss.nttdata.com> Message-ID: <88c451c7-2729-4329-fbae-6ed7664adea1@oss.nttdata.com> Date: Mon, 25 Jan 2021 23:44:56 +0900 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:78.0) Gecko/20100101 Thunderbird/78.6.1 MIME-Version: 1.0 In-Reply-To: <8002219d-b999-12fa-2327-49afe40e5fbb@oss.nttdata.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk On 2021/01/22 18:11, Fujii Masao wrote: > > > On 2021/01/22 14:37, torikoshia wrote: >> On 2021-01-21 12:48, Fujii Masao wrote: >> >>> Thanks for updating the patch! I think that this is really useful feature!! >> >> Thanks for reviewing! >> >>> I have two minor comments. >>> >>> + >>> + wait_start timestamptz >>> >>> The column name "wait_start" should be "waitstart" for the sake of consistency >>> with other column names in pg_locks? pg_locks seems to avoid including >>> an underscore in column names, so "locktype" is used instead of "lock_type", >>> "virtualtransaction" is used instead of "virtual_transaction", etc. >>> >>> + Lock acquisition wait start time. NULL if >>> + lock acquired. >>> >> >> Agreed. >> >> I also changed the variable name "wait_start" in struct PGPROC and >> LockInstanceData to "waitStart" for the same reason. >> >> >>> There seems the case where the wait start time is NULL even when "grant" >>> is false. It's better to add note about that case into the docs? For example, >>> I found that the wait start time is NULL while the startup process is waiting >>> for the lock. Is this only that case? >> >> Thanks, this is because I set 'waitstart' in the following >> condition. >> >> ---src/backend/storage/lmgr/proc.c >> > 1250 if (!InHotStandby) >> >> As far as considering this, I guess startup process would >> be the only case. >> >> I also think that in case of startup process, it seems possible >> to set 'waitstart' in ResolveRecoveryConflictWithLock(), so I >> did it in the attached patch. > > This change seems to cause "waitstart" to be reset every time > ResolveRecoveryConflictWithLock() is called in the do-while loop. > I guess this is not acceptable. Right? > > To avoid that issue, IMO the following change is better. Thought? > > - else if (log_recovery_conflict_waits) > + else > { > + TimestampTz now = GetCurrentTimestamp(); > + > + MyProc->waitStart = now; > + > /* > * Set the wait start timestamp if logging is enabled and in hot > * standby. > */ > - standbyWaitStart = GetCurrentTimestamp(); > + if (log_recovery_conflict_waits) > + standbyWaitStart = now > } > > This change causes the startup process to call GetCurrentTimestamp() > additionally even when log_recovery_conflict_waits is disabled. Which > might decrease the performance of the startup process, but that performance > degradation can happen only when the startup process waits in > ACCESS EXCLUSIVE lock. So if this my understanding right, IMO it's almost > harmless to call GetCurrentTimestamp() additionally in that case. Thought? According to the off-list discussion with you, this should not happen because ResolveRecoveryConflictWithDatabase() sets MyProc->waitStart only when it's not set yet (i.e., = 0). That's good. So I'd withdraw my comment. + if (MyProc->waitStart == 0) + MyProc->waitStart = now; + MyProc->waitStart = get_timeout_start_time(DEADLOCK_TIMEOUT); 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. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION