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