From: Fujii Masao <masao.fujii@oss.nttdata.com>
To: torikoshia <torikoshia@oss.nttdata.com>
Cc: 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: Tue, 16 Feb 2021 16:59:08 +0900
Message-ID: <1df88660-6f08-cc6e-b7e2-f85296a2bdab@oss.nttdata.com> (raw)
In-Reply-To: <655f1a5b-4fa8-53ba-c3e2-8cee31587d37@oss.nttdata.com>
References: <a96013dc51cdc56b2a2b84fa8a16a993@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>
<ef0dd522-9096-a60f-a0fa-5f0a0fdbafbd@oss.nttdata.com>
<3db333a9-31f7-a225-8038-eeb835ba8f37@oss.nttdata.com>
<b8425207811a6b99d0ecd76844906a8e@oss.nttdata.com>
<f77120a3-4762-bf71-57d5-f0be081715f7@oss.nttdata.com>
<d45e44d655f2ba06d9e722d3dbcfdd79@oss.nttdata.com>
<990e5d4b-075f-101f-aec5-bb44f9b30550@oss.nttdata.com>
<40dfaa75-1058-e811-1f7c-4cf7203a3068@oss.nttdata.com>
<64102638-7fc3-1941-12f1-e139e3463c03@oss.nttdata.com>
<067d5597e5de82fe47e5716991ab65c4@oss.nttdata.com>
<337d5311-6113-15e0-bd85-90b47b55f5ae@oss.nttdata.com>
<655f1a5b-4fa8-53ba-c3e2-8cee31587d37@oss.nttdata.com>
On 2021/02/15 15:17, Fujii Masao wrote:
>
>
> On 2021/02/10 10:43, Fujii Masao wrote:
>>
>>
>> On 2021/02/09 23:31, torikoshia wrote:
>>> On 2021-02-09 22:54, Fujii Masao wrote:
>>>> On 2021/02/09 19:11, Fujii Masao wrote:
>>>>>
>>>>>
>>>>> On 2021/02/09 18:13, Fujii Masao wrote:
>>>>>>
>>>>>>
>>>>>> 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 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.
>>>>>>>
>>>>>>> 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.
>>>>>
>>>>> But I reverted the patch because buildfarm members rorqual and
>>>>> prion don't like the patch. I'm trying to investigate the cause
>>>>> of this failures.
>>>>>
>>>>> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=rorqual&dt=2021-02-09%2009%3A20%3A10
>>>>
>>>> - relation | locktype | mode
>>>> ------------------+----------+---------------------
>>>> - test_prepared_1 | relation | RowExclusiveLock
>>>> - test_prepared_1 | relation | AccessExclusiveLock
>>>> -(2 rows)
>>>> -
>>>> +ERROR: invalid spinlock number: 0
>>>>
>>>> "rorqual" reported that the above error happened in the server built with
>>>> --disable-atomics --disable-spinlocks when reading pg_locks after
>>>> the transaction was prepared. The cause of this issue is that "waitStart"
>>>> atomic variable in the dummy proc created at the end of prepare transaction
>>>> was not initialized. I updated the patch so that pg_atomic_init_u64() is
>>>> called for the "waitStart" in the dummy proc for prepared transaction.
>>>> Patch attached. I confirmed that the patched server built with
>>>> --disable-atomics --disable-spinlocks passed all the regression tests.
>>>
>>> Thanks for fixing the bug, I also tested v9.patch configured with
>>> --disable-atomics --disable-spinlocks on my environment and confirmed
>>> that all tests have passed.
>>
>> Thanks for the test!
>>
>> I found another bug in the patch. InitProcess() initializes "waitStart",
>> but previously InitAuxiliaryProcess() did not. This could cause "invalid
>> spinlock number" error when reading pg_locks in the standby server.
>> I fixed that. Attached is the updated version of the patch.
>
> I pushed this version. Thanks!
While reading the patch again, I found two minor things.
1. As discussed in another thread [1], the atomic variable "waitStart" should
be initialized at the postmaster startup rather than the startup of each
child process. I changed "waitStart" so that it's initialized in
InitProcGlobal() and also reset to 0 by using pg_atomic_write_u64() in
InitProcess() and InitAuxiliaryProcess().
2. Thanks to the above change, InitProcGlobal() initializes "waitStart"
even in PGPROC entries for prepare transactions. But those entries are
zeroed in MarkAsPreparingGuts(), so "waitStart" needs to be initialized
again. Currently TwoPhaseGetDummyProc() initializes "waitStart" in the
PGPROC entry for prepare transaction. But it's better to do that in
MarkAsPreparingGuts() instead because that function initializes other
PGPROC variables. So I moved that initialization code from
TwoPhaseGetDummyProc() to MarkAsPreparingGuts().
Patch attached. Thought?
[1] https://postgr.es/m/7ef8708c-5b6b-edd3-2cf2-7783f1c7c175@oss.nttdata.com
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.cindex 70d22577ce..c9b13062b0 100644--- a/src/backend/access/transam/twophase.c+++ b/src/backend/access/transam/twophase.c@@ -475,6 +475,7 @@ MarkAsPreparingGuts(GlobalTransaction gxact, TransactionId xid, const char *gid,
proc->lwWaitMode = 0;
proc->waitLock = NULL;
proc->waitProcLock = NULL;
+ pg_atomic_init_u64(&proc->waitStart, 0);
for (i = 0; i < NUM_LOCK_PARTITIONS; i++)
SHMQueueInit(&(proc->myProcLocks[i]));
/* subxid data must be filled later by GXactLoadSubxactData */
@@ -873,15 +874,8 @@ PGPROC *
TwoPhaseGetDummyProc(TransactionId xid, bool lock_held)
{
GlobalTransaction gxact = TwoPhaseGetGXact(xid, lock_held);
- PGPROC *dummy = &ProcGlobal->allProcs[gxact->pgprocno];- /*- * Initialize atomic variable in dummy proc so that GetLockStatusData()- * can read it later.- */- pg_atomic_init_u64(&dummy->waitStart, 0);-- return dummy;+ return &ProcGlobal->allProcs[gxact->pgprocno];
}
/************************************************************************/
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.cindex 0884909a22..8e7d21456a 100644--- a/src/backend/storage/lmgr/proc.c+++ b/src/backend/storage/lmgr/proc.c@@ -281,6 +281,7 @@ InitProcGlobal(void)
*/
pg_atomic_init_u32(&(procs[i].procArrayGroupNext), INVALID_PGPROCNO);
pg_atomic_init_u32(&(procs[i].clogGroupNext), INVALID_PGPROCNO);
+ pg_atomic_init_u64(&(procs[i].waitStart), 0);
}
/*
@@ -402,7 +403,7 @@ InitProcess(void)
MyProc->lwWaitMode = 0;
MyProc->waitLock = NULL;
MyProc->waitProcLock = NULL;
- pg_atomic_init_u64(&MyProc->waitStart, 0);+ pg_atomic_write_u64(&MyProc->waitStart, 0);
#ifdef USE_ASSERT_CHECKING
{
int i;
@@ -581,7 +582,7 @@ InitAuxiliaryProcess(void)
MyProc->lwWaitMode = 0;
MyProc->waitLock = NULL;
MyProc->waitProcLock = NULL;
- pg_atomic_init_u64(&MyProc->waitStart, 0);+ pg_atomic_write_u64(&MyProc->waitStart, 0);
#ifdef USE_ASSERT_CHECKING
{
int i;
Attachments:
[text/plain] bugfix_pg_locks_waitstart.patch (2.0K, ../1df88660-6f08-cc6e-b7e2-f85296a2bdab@oss.nttdata.com/2-bugfix_pg_locks_waitstart.patch)
download | inline diff:diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.cindex 70d22577ce..c9b13062b0 100644--- a/src/backend/access/transam/twophase.c+++ b/src/backend/access/transam/twophase.c@@ -475,6 +475,7 @@ MarkAsPreparingGuts(GlobalTransaction gxact, TransactionId xid, const char *gid,
proc->lwWaitMode = 0;
proc->waitLock = NULL;
proc->waitProcLock = NULL;
+ pg_atomic_init_u64(&proc->waitStart, 0);
for (i = 0; i < NUM_LOCK_PARTITIONS; i++)
SHMQueueInit(&(proc->myProcLocks[i]));
/* subxid data must be filled later by GXactLoadSubxactData */
@@ -873,15 +874,8 @@ PGPROC *
TwoPhaseGetDummyProc(TransactionId xid, bool lock_held)
{
GlobalTransaction gxact = TwoPhaseGetGXact(xid, lock_held);
- PGPROC *dummy = &ProcGlobal->allProcs[gxact->pgprocno];- /*- * Initialize atomic variable in dummy proc so that GetLockStatusData()- * can read it later.- */- pg_atomic_init_u64(&dummy->waitStart, 0);-- return dummy;+ return &ProcGlobal->allProcs[gxact->pgprocno];
}
/************************************************************************/
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.cindex 0884909a22..8e7d21456a 100644--- a/src/backend/storage/lmgr/proc.c+++ b/src/backend/storage/lmgr/proc.c@@ -281,6 +281,7 @@ InitProcGlobal(void)
*/
pg_atomic_init_u32(&(procs[i].procArrayGroupNext), INVALID_PGPROCNO);
pg_atomic_init_u32(&(procs[i].clogGroupNext), INVALID_PGPROCNO);
+ pg_atomic_init_u64(&(procs[i].waitStart), 0);
}
/*
@@ -402,7 +403,7 @@ InitProcess(void)
MyProc->lwWaitMode = 0;
MyProc->waitLock = NULL;
MyProc->waitProcLock = NULL;
- pg_atomic_init_u64(&MyProc->waitStart, 0);+ pg_atomic_write_u64(&MyProc->waitStart, 0);
#ifdef USE_ASSERT_CHECKING
{
int i;
@@ -581,7 +582,7 @@ InitAuxiliaryProcess(void)
MyProc->lwWaitMode = 0;
MyProc->waitLock = NULL;
MyProc->waitProcLock = NULL;
- pg_atomic_init_u64(&MyProc->waitStart, 0);+ pg_atomic_write_u64(&MyProc->waitStart, 0);
#ifdef USE_ASSERT_CHECKING
{
int i;
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: masao.fujii@oss.nttdata.com, 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: <1df88660-6f08-cc6e-b7e2-f85296a2bdab@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