Hi,
On 8/17/23 9:37 AM, Masahiro Ikeda wrote:
> Hi,
>
> On 2023-08-17 14:53, Drouvot, Bertrand wrote:
>
> 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
Okay, using the plural form in v8 attached.
>
> 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: $!";
>
Nice catch, thanks, fixed in v8.
>
> 3)
>
> "List all the wait events type, name and descriptions" is enough?
>
> + * List all the wait events (type, name) and their descriptions.
>
Agree, used "List all the wait events type, name and description" in v8
(using description in singular as compared to your proposal)
> 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_eventw ON (a.wait_event_type = w.type AND a.wait_event = w.name) WHERE wait_event is NOT NULL;
Agree, I think that's a good idea.
I'd prefer to add also a filtering on "state='active'" for this example (I think that would provide
a "real" use case as the sessions are not waiting if they are not active).
Done in v8.
> 5)
>
> Though I'm not familiar the value, do we need procost?
>
I think it makes sense to add a "small" one as it's done.
BTW, while looking at it, I changed prorows to a more "accurate"
value.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com