From: torikoshia <torikoshia@oss.nttdata.com>
To: Fujii Masao <masao.fujii@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, 09 Feb 2021 23:31:10 +0900
Message-ID: <067d5597e5de82fe47e5716991ab65c4@oss.nttdata.com> (raw)
In-Reply-To: <64102638-7fc3-1941-12f1-e139e3463c03@oss.nttdata.com>
References: <a96013dc51cdc56b2a2b84fa8a16a993@oss.nttdata.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>
<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>
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.
>
> BTW, while investigating this issue, I found that pg_stat_wal_receiver
> also
> could cause this error even in the current master (without the patch).
> I will report that in separate thread.
>
>
>> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prion&dt=2021-02-09%2009%3A13%3A16
>
> "prion" reported the following error. But I'm not sure how the changes
> of
> pg_locks caused this error. I found that Heikki also reported at [1]
> that
> "prion" failed with the same error but was not sure how it happened.
> This makes me think for now that this issue is not directly related to
> the pg_locks changes.
Thanks! I was wondering how these errors were related to the commit.
Regards,
--
Atsushi Torikoshi
> -------------------------------------
> pg_dump: error: query failed: ERROR: missing chunk number 0 for toast
> value 14444 in pg_toast_2619
> pg_dump: error: query was: SELECT
> a.attnum,
> a.attname,
> a.atttypmod,
> a.attstattarget,
> a.attstorage,
> t.typstorage,
> a.attnotnull,
> a.atthasdef,
> a.attisdropped,
> a.attlen,
> a.attalign,
> a.attislocal,
> pg_catalog.format_type(t.oid, a.atttypmod) AS atttypname,
> array_to_string(a.attoptions, ', ') AS attoptions,
> CASE WHEN a.attcollation <> t.typcollation THEN a.attcollation ELSE 0
> END AS attcollation,
> pg_catalog.array_to_string(ARRAY(SELECT
> pg_catalog.quote_ident(option_name) || ' ' ||
> pg_catalog.quote_literal(option_value) FROM
> pg_catalog.pg_options_to_table(attfdwoptions) ORDER BY option_name),
> E',
> ') AS attfdwoptions,
> a.attidentity,
> CASE WHEN a.atthasmissing AND NOT a.attisdropped THEN a.attmissingval
> ELSE null END AS attmissingval,
> a.attgenerated
> FROM pg_catalog.pg_attribute a LEFT JOIN pg_catalog.pg_type t ON
> a.atttypid = t.oid
> WHERE a.attrelid = '35987'::pg_catalog.oid AND a.attnum >
> 0::pg_catalog.int2
> ORDER BY a.attnum
> pg_dumpall: error: pg_dump failed on database "regression", exiting
> waiting for server to shut down.... done
> server stopped
> pg_dumpall of post-upgrade database cluster failed
> -------------------------------------
>
> [1]
> https://www.postgresql.org/message-id/f03ea04a-9b77-e371-9ab9-182cb35db1f9@iki.fi
>
>
> Regards,
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, masao.fujii@oss.nttdata.com, barwick@gmail.com, robertmhaas@gmail.com, pryzby@telsasoft.com
Subject: Re: adding wait_start column to pg_locks
In-Reply-To: <067d5597e5de82fe47e5716991ab65c4@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