From: Masahiro Ikeda <ikedamsh@oss.nttdata.com>
To: Drouvot, Bertrand <bertranddrouvot.pg@gmail.com>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: tgl@sss.pgh.pa.us
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: WIP: new system catalog pg_wait_event
Date: Thu, 17 Aug 2023 16:37:22 +0900
Message-ID: <393fe340ab6980f399402a7dfbe13c4c@oss.nttdata.com> (raw)
In-Reply-To: <d28ad85d-1deb-4648-92a0-f38273fc91ca@gmail.com>
References: <0109438a-9672-322b-7638-04ad47c97d0f@gmail.com>
<1dc98591-0d06-f0da-bc88-21f529d6fb8e@gmail.com>
<20230808.115332.301701413576096139.horikyota.ntt@gmail.com>
<ZNGw9T2t5z5Kg1xd@paquier.xyz>
<b21e5706-b6dd-42f1-a744-3afffe87c2c9@gmail.com>
<c915434a-3d3a-4135-8f3a-fca0cf22c6ae@gmail.com>
<ZNNGq7DTv9ur5113@paquier.xyz>
<b71d1977-1a8b-4129-8c03-533031dbd004@gmail.com>
<ZNmvegQirGI9ifVw@paquier.xyz>
<2f84b397-d020-493c-924c-610b9dd18f8d@gmail.com>
<ZNxrMdmgrlovw1qD@paquier.xyz>
<f2204399-cfdb-40e7-9479-9373ec6ba0ab@gmail.com>
<735fbd560ae914c96faaa23cc8d9a118@oss.nttdata.com>
<d28ad85d-1deb-4648-92a0-f38273fc91ca@gmail.com>
Hi,
On 2023-08-17 14:53, Drouvot, Bertrand wrote:
> On 8/17/23 3:53 AM, Masahiro Ikeda wrote:
>> 1)
>>
>> The regular expression needs to be changed in
>> generate-wait_event_types.pl.
>> I have compared the documentation with the output of the pg_wait_event
>> view and found the following differences.
>>
>> * For parameter names, the substitution for underscore is missing.
>> -ArchiveCleanupCommand Waiting for archive_cleanup_command to complete
>> -ArchiveCommand Waiting for archive_command to complete
>> +ArchiveCleanupCommand Waiting for archive-cleanup-command to complete
>> +ArchiveCommand Waiting for archive-command to complete
>> -RecoveryEndCommand Waiting for recovery_end_command to complete
>> +RecoveryEndCommand Waiting for recovery-end-command to complete
>> -RestoreCommand Waiting for restore_command to complete
>> +RestoreCommand Waiting for restore-command to complete
>>
>
> Yeah, nice catch. v7 just shared up-thread replace "-" by "_"
> for such a case. But I'm not sure the pg_wait_event description field
> needs
> to be 100% aligned with the documentation: wouldn't be better to
> replace
> "-" by " " in such cases in pg_wait_event?
>
>> * The HTML tag match is not shortest match.
>> -frozenid Waiting to update pg_database.datfrozenxid and
>> pg_database.datminmxid
>> +frozenid Waiting to update datminmxid
>>
>
> Nice catch, fixed in v7.
Thanks, I confirmed.
>> * There are two blanks before "about".
>
> This is coming from wait_event_names.txt, thanks for pointing out.
> I just submitted a patch [1] to fix that.
Thanks. I agree it's better to be treated as a separated patch.
>> Also " for heavy weight is
>> removed (is it intended?)
>> -LockManager Waiting to read or update information about "heavyweight"
>> locks
>> +LockManager Waiting to read or update information about heavyweight
>> locks
>>
>
> Not intended, fixed in v7.
OK, I confirmed it's fixed.
>> * Do we need "worker_spi_main" in the description? The name column
>> shows the same one, so it could be omitted.
>>> pg_wait_event
>>> worker_spi_main Custom wait event "worker_spi_main" defined by
>>> extension
>>
>
> Do you mean remove the wait event name from the description in case of
> custom
> extension wait events? I'd prefer to keep it, if not the descriptions
> would be
> all the same for custom wait events.
OK, I thought it's redundant.
BTW, is it better to start with "Waiting" like any other lines?
For example, "Waiting for custom event \"worker_spi_main\" defined by an
extension".
>> 2)
>>
>> Would it be better to add "extension" meaning unassigned?
>>
>>> Extensions can add Extension and LWLock types to the list shown in
>>> Table 28.8 and Table 28.12. In some cases, the name of LWLock
>>> assigned by an extension will not be available in all server
>>> processes; It might be reported as just “extension” rather than the
>>> extension-assigned name.
>>
>
> Yeah, could make sense but I think that should be a dedicated patch
> for the documentation.
OK, make sense.
>> 3)
>>
>> Would index == els be better for the following Assert?
>> + Assert(index <= els);
>>
>
> Agree, done in v7.
>
>> 4)
>>
>> There is a typo. alll -> all
>> + /* Allocate enough space for alll entries */
Thanks, I confirmed it's fixed.
The followings are additional comments for v7.
1)
>> I am not sure that "pg_wait_event" is a good idea for the name if the
>> new view. How about "pg_wait_events" instead, in plural form? There
>> is more than one wait event listed.
> I'd prefer the singular form. There is a lot of places where it's
> already used
> (pg_database, pg_user, pg_namespace...to name a few) and it looks like
> that using
> the plural form are exceptions.
Since I got the same feeling as Michael-san that "pg_wait_events" would
be better,
I check the names of all system views. I think the singular form seems
to be
exceptions for views though the singular form is used usually for system
catalogs.
https://www.postgresql.org/docs/devel/views.htmlhttps://www.postgresql.org/docs/devel/catalogs.html
2)
$ctmp must be $wctmp?
+ rename($wctmp, "$output_path/wait_event_funcs_data.c")
+ || die "rename: $ctmp to $output_path/wait_event_funcs_data.c: $!";
3)
"List all the wait events type, name and descriptions" is enough?
+ * List all the wait events (type, name) and their descriptions.
4)
Why don't you add the usage of the view in the monitoring.sgml?
```
<para>
Here is an example of how wait events can be viewed:
<programlisting>
SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE
wait_event is NOT NULL;
pid | wait_event_type | wait_event
------+-----------------+------------
2540 | Lock | relation
6644 | LWLock | ProcArray
(2 rows)
</programlisting>
</para>
```
ex.
postgres(3789417)=# SELECT a.pid, a.wait_event_type, a.wait_event,
w.description FROM pg_stat_activity a JOIN pg_wait_event w ON
(a.wait_event_type = w.type AND a.wait_event = w.name) WHERE wait_event
is NOT NULL;
pid | wait_event_type | wait_event |
description
---------+-----------------+-------------------+--------------------------------------------------------
3783759 | Activity | AutoVacuumMain | Waiting in main loop of
autovacuum launcher process
3812866 | Extension | WorkerSpiMain | Custom wait event
"WorkerSpiMain" defined by extension
3783756 | Activity | BgWriterHibernate | Waiting in background
writer process, hibernating
3783760 | Activity | ArchiverMain | Waiting in main loop of
archiver process
3783755 | Activity | CheckpointerMain | Waiting in main loop of
checkpointer process
3783758 | Activity | WalWriterMain | Waiting in main loop of
WAL writer process
(6 rows)
5)
Though I'm not familiar the value, do we need procost?
+{ oid => '8403', descr => 'describe wait events',
+ proname => 'pg_get_wait_events', procost => '10', prorows => '100',
+ proretset => 't', provolatile => 's', prorettype => 'record',
+ proargtypes => '', proallargtypes => '{text,text,text}',
+ proargmodes => '{o,o,o}', proargnames => '{type,name,description}',
+ prosrc => 'pg_get_wait_events' },
Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION
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: ikedamsh@oss.nttdata.com, bertranddrouvot.pg@gmail.com, michael@paquier.xyz, horikyota.ntt@gmail.com, tgl@sss.pgh.pa.us, pgsql-hackers@lists.postgresql.org
Subject: Re: WIP: new system catalog pg_wait_event
In-Reply-To: <393fe340ab6980f399402a7dfbe13c4c@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