Re: WIP: new system catalog pg_wait_event

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