From: torikoshia <torikoshia@oss.nttdata.com>
To: 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: Mon, 18 Jan 2021 12:00:22 +0900
Message-ID: <0fd375a53e306566cea7f451cc8cfcde@oss.nttdata.com> (raw)
In-Reply-To: <23d39ee9c31643fad8f9ba9c5cf3aaf4@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>
On 2021-01-15 15:23, torikoshia wrote:
> Thanks for your reviewing and comments!
>
> On 2021-01-14 12:39, Ian Lawrence Barwick wrote:
>> Looking at the code, this happens as the wait start time is being
>> recorded in
>> the lock record itself, so always contains the value reported by the
>> latest lock
>> acquisition attempt.
>
> I think you are right and wait_start should not be recorded
> in the LOCK.
>
>
> On 2021-01-15 11:48, Ian Lawrence Barwick wrote:
>> 2021年1月15日(金) 3:45 Robert Haas <robertmhaas@gmail.com>:
>>
>>> On Wed, Jan 13, 2021 at 10:40 PM Ian Lawrence Barwick
>>> <barwick@gmail.com> wrote:
>>>> It looks like the logical place to store the value is in the
>>> PROCLOCK
>>>> structure; ...
>>>
>>> That seems surprising, because there's one PROCLOCK for every
>>> combination of a process and a lock. But, a process can't be waiting
>>> for more than one lock at the same time, because once it starts
>>> waiting to acquire the first one, it can't do anything else, and
>>> thus
>>> can't begin waiting for a second one. So I would have thought that
>>> this would be recorded in the PROC.
>>
>> Umm, I think we're at cross-purposes here. The suggestion is to note
>> the time when the process started waiting for the lock in the
>> process's
>> PROCLOCK, rather than in the lock itself (which in the original
>> version
>> of the patch resulted in all processes with an interest in the lock
>> appearing
>> to have been waiting to acquire it since the time a lock acquisition
>> was most recently attempted).
>
> AFAIU, it seems possible to record wait_start in the PROCLOCK but
> redundant since each process can wait at most one lock.
>
> To confirm my understanding, I'm going to make another patch that
> records wait_start in the PGPROC.
Attached a patch.
I noticed previous patches left the wait_start untouched even after
it acquired lock.
The patch also fixed it.
Any thoughts?
Regards,
--
Atsushi Torikoshi
Attachments:
[text/x-diff] v4-0001-To-examine-the-duration-of-locks-we-did-join-on-p.patch (9.5K, ../0fd375a53e306566cea7f451cc8cfcde@oss.nttdata.com/2-v4-0001-To-examine-the-duration-of-locks-we-did-join-on-p.patch)
download | inline diff:
From 62ff3e4dba7d45c260a62a33425cb2d1e6b822c9 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <torikoshia@oss.nttdata.com>
Date: Mon, 18 Jan 2021 10:01:35 +0900
Subject: [PATCH v4] To examine the duration of locks, we did join on pg_locks
and pg_stat_activity and used columns such as query_start or state_change.
However, since they are the moment when queries have started or their state
has changed, we could not get the exact lock duration in this way.
This patch adds a new field preserving the time at which locks started
waiting.
---
contrib/amcheck/expected/check_btree.out | 4 ++--
doc/src/sgml/catalogs.sgml | 10 ++++++++++
src/backend/storage/lmgr/lock.c | 8 ++++++++
src/backend/storage/lmgr/proc.c | 4 ++++
src/backend/utils/adt/lockfuncs.c | 9 ++++++++-
src/include/catalog/pg_proc.dat | 6 +++---
src/include/storage/lock.h | 2 ++
src/include/storage/proc.h | 1 +
src/test/regress/expected/rules.out | 5 +++--
9 files changed, 41 insertions(+), 8 deletions(-)
diff --git a/contrib/amcheck/expected/check_btree.out b/contrib/amcheck/expected/check_btree.outindex 13848b7449..c0aecb0288 100644--- a/contrib/amcheck/expected/check_btree.out+++ b/contrib/amcheck/expected/check_btree.out@@ -97,8 +97,8 @@ SELECT bt_index_parent_check('bttest_b_idx');
SELECT * FROM pg_locks
WHERE relation = ANY(ARRAY['bttest_a', 'bttest_a_idx', 'bttest_b', 'bttest_b_idx']::regclass[])
AND pid = pg_backend_pid();
- locktype | database | relation | page | tuple | virtualxid | transactionid | classid | objid | objsubid | virtualtransaction | pid | mode | granted | fastpath -----------+----------+----------+------+-------+------------+---------------+---------+-------+----------+--------------------+-----+------+---------+----------+ locktype | database | relation | page | tuple | virtualxid | transactionid | classid | objid | objsubid | virtualtransaction | pid | mode | granted | fastpath | wait_start +----------+----------+----------+------+-------+------------+---------------+---------+-------+----------+--------------------+-----+------+---------+----------+------------
(0 rows)
COMMIT;
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgmlindex 43d7a1ad90..a5ce0835a9 100644--- a/doc/src/sgml/catalogs.sgml+++ b/doc/src/sgml/catalogs.sgml@@ -10589,6 +10589,16 @@ SCRAM-SHA-256$<replaceable><iteration count></replaceable>:<replaceable>&l
lock table
</para></entry>
</row>
++ <row>+ <entry role="catalog_table_entry"><para role="column_definition">+ <structfield>wait_start</structfield> <type>timestamptz</type>+ </para>+ <para>+ Lock acquisition wait start time. <literal>NULL</literal> if+ lock acquired.+ </para></entry>+ </row>
</tbody>
</tgroup>
</table>
diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.cindex 20e50247ea..5b5fb474e0 100644--- a/src/backend/storage/lmgr/lock.c+++ b/src/backend/storage/lmgr/lock.c@@ -3627,6 +3627,12 @@ GetLockStatusData(void)
instance->leaderPid = proc->pid;
instance->fastpath = true;
+ /*+ * Successfully taking fast path lock means there were no+ * conflicting locks.+ */+ instance->wait_start = 0;+
el++;
}
@@ -3654,6 +3660,7 @@ GetLockStatusData(void)
instance->pid = proc->pid;
instance->leaderPid = proc->pid;
instance->fastpath = true;
+ instance->wait_start = 0;
el++;
}
@@ -3706,6 +3713,7 @@ GetLockStatusData(void)
instance->pid = proc->pid;
instance->leaderPid = proclock->groupLeader->pid;
instance->fastpath = false;
+ instance->wait_start = proc->wait_start;
el++;
}
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.cindex c87ffc6549..34173a5fdb 100644--- a/src/backend/storage/lmgr/proc.c+++ b/src/backend/storage/lmgr/proc.c@@ -402,6 +402,7 @@ InitProcess(void)
MyProc->lwWaitMode = 0;
MyProc->waitLock = NULL;
MyProc->waitProcLock = NULL;
+ MyProc->wait_start = 0;
#ifdef USE_ASSERT_CHECKING
{
int i;
@@ -1262,6 +1263,8 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable)
}
else
enable_timeout_after(DEADLOCK_TIMEOUT, DeadlockTimeout);
++ MyProc->wait_start = get_timeout_start_time(DEADLOCK_TIMEOUT);
}
else if (log_recovery_conflict_waits)
{
@@ -1678,6 +1681,7 @@ ProcWakeup(PGPROC *proc, ProcWaitStatus waitStatus)
proc->waitLock = NULL;
proc->waitProcLock = NULL;
proc->waitStatus = waitStatus;
+ proc->wait_start = 0;
/* And awaken it */
SetLatch(&proc->procLatch);
diff --git a/src/backend/utils/adt/lockfuncs.c b/src/backend/utils/adt/lockfuncs.cindex b1cf5b79a7..e26595bd05 100644--- a/src/backend/utils/adt/lockfuncs.c+++ b/src/backend/utils/adt/lockfuncs.c@@ -63,7 +63,7 @@ typedef struct
} PG_Lock_Status;
/* Number of columns in pg_locks output */
-#define NUM_LOCK_STATUS_COLUMNS 15+#define NUM_LOCK_STATUS_COLUMNS 16
/*
* VXIDGetDatum - Construct a text representation of a VXID
@@ -142,6 +142,8 @@ pg_lock_status(PG_FUNCTION_ARGS)
BOOLOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 15, "fastpath",
BOOLOID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 16, "wait_start",+ TIMESTAMPTZOID, -1, 0);
funcctx->tuple_desc = BlessTupleDesc(tupdesc);
@@ -336,6 +338,10 @@ pg_lock_status(PG_FUNCTION_ARGS)
values[12] = CStringGetTextDatum(GetLockmodeName(instance->locktag.locktag_lockmethodid, mode));
values[13] = BoolGetDatum(granted);
values[14] = BoolGetDatum(instance->fastpath);
+ if (!granted && instance->wait_start != 0)+ values[15] = TimestampTzGetDatum(instance->wait_start);+ else+ nulls[15] = true;
tuple = heap_form_tuple(funcctx->tuple_desc, values, nulls);
result = HeapTupleGetDatum(tuple);
@@ -406,6 +412,7 @@ pg_lock_status(PG_FUNCTION_ARGS)
values[12] = CStringGetTextDatum("SIReadLock");
values[13] = BoolGetDatum(true);
values[14] = BoolGetDatum(false);
+ nulls[15] = true;
tuple = heap_form_tuple(funcctx->tuple_desc, values, nulls);
result = HeapTupleGetDatum(tuple);
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.datindex d27336adcd..5cd93c4587 100644--- a/src/include/catalog/pg_proc.dat+++ b/src/include/catalog/pg_proc.dat@@ -5948,9 +5948,9 @@
{ oid => '1371', descr => 'view system lock information',
proname => 'pg_lock_status', prorows => '1000', proretset => 't',
provolatile => 'v', prorettype => 'record', proargtypes => '',
- proallargtypes => '{text,oid,oid,int4,int2,text,xid,oid,oid,int2,text,int4,text,bool,bool}',- proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}',- proargnames => '{locktype,database,relation,page,tuple,virtualxid,transactionid,classid,objid,objsubid,virtualtransaction,pid,mode,granted,fastpath}',+ proallargtypes => '{text,oid,oid,int4,int2,text,xid,oid,oid,int2,text,int4,text,bool,bool,timestamptz}',+ proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}',+ proargnames => '{locktype,database,relation,page,tuple,virtualxid,transactionid,classid,objid,objsubid,virtualtransaction,pid,mode,granted,fastpath,wait_start}',
prosrc => 'pg_lock_status' },
{ oid => '2561',
descr => 'get array of PIDs of sessions blocking specified backend PID from acquiring a heavyweight lock',
diff --git a/src/include/storage/lock.h b/src/include/storage/lock.hindex 2e6ef174e9..e37b7a66e9 100644--- a/src/include/storage/lock.h+++ b/src/include/storage/lock.h@@ -22,6 +22,7 @@
#include "storage/lockdefs.h"
#include "storage/lwlock.h"
#include "storage/shmem.h"
+#include "utils/timestamp.h"
/* struct PGPROC is declared in proc.h, but must forward-reference it */
typedef struct PGPROC PGPROC;
@@ -445,6 +446,7 @@ typedef struct LockInstanceData
LOCKMODE waitLockMode; /* lock awaited by this PGPROC, if any */
BackendId backend; /* backend ID of this PGPROC */
LocalTransactionId lxid; /* local transaction ID of this PGPROC */
+ TimestampTz wait_start; /* time at which this PGPROC started waiting for lock */
int pid; /* pid of this PGPROC */
int leaderPid; /* pid of group leader; = pid if no group */
bool fastpath; /* taken via fastpath? */
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.hindex 683ab64f76..dfbc34163f 100644--- a/src/include/storage/proc.h+++ b/src/include/storage/proc.h@@ -181,6 +181,7 @@ struct PGPROC
LOCKMODE waitLockMode; /* type of lock we're waiting for */
LOCKMASK heldLocks; /* bitmask for lock types already held on this
* lock object by this backend */
+ TimestampTz wait_start; /* time at which wait for lock acquisition started */
bool delayChkpt; /* true if this proc delays checkpoint start */
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.outindex a687e99d1e..8ccc859882 100644--- a/src/test/regress/expected/rules.out+++ b/src/test/regress/expected/rules.out@@ -1394,8 +1394,9 @@ pg_locks| SELECT l.locktype,
l.pid,
l.mode,
l.granted,
- l.fastpath- FROM pg_lock_status() l(locktype, database, relation, page, tuple, virtualxid, transactionid, classid, objid, objsubid, virtualtransaction, pid, mode, granted, fastpath);+ l.fastpath,+ l.wait_start+ FROM pg_lock_status() l(locktype, database, relation, page, tuple, virtualxid, transactionid, classid, objid, objsubid, virtualtransaction, pid, mode, granted, fastpath, wait_start);
pg_matviews| SELECT n.nspname AS schemaname,
c.relname AS matviewname,
pg_get_userbyid(c.relowner) AS matviewowner,
--
2.18.1
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, barwick@gmail.com, robertmhaas@gmail.com, pryzby@telsasoft.com
Subject: Re: adding wait_start column to pg_locks
In-Reply-To: <0fd375a53e306566cea7f451cc8cfcde@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