Re: WIP: new system catalog pg_wait_event

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