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.html
https://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
