MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit
Date: Thu, 17 Aug 2023 16:37:22 +0900
From: Masahiro Ikeda <ikedamsh@oss.nttdata.com>
To: "Drouvot, Bertrand" <bertranddrouvot.pg@gmail.com>
Cc: Michael Paquier <michael@paquier.xyz>, Kyotaro Horiguchi
 <horikyota.ntt@gmail.com>, tgl@sss.pgh.pa.us,
 pgsql-hackers@lists.postgresql.org
Subject: Re: WIP: new system catalog pg_wait_event
In-Reply-To: <d28ad85d-1deb-4648-92a0-f38273fc91ca@gmail.com>
References: <0109438a-9672-322b-7638-04ad47c97d0f@gmail.com>
 <1dc98591-0d06-f0da-bc88-21f529d6fb8e@gmail.com>
 <20230808.115332.301701413576096139.horikyota.ntt@gmail.com>
 <ZNGw9T2t5z5Kg1xd@paquier.xyz>
 <b21e5706-b6dd-42f1-a744-3afffe87c2c9@gmail.com>
 <c915434a-3d3a-4135-8f3a-fca0cf22c6ae@gmail.com>
 <ZNNGq7DTv9ur5113@paquier.xyz>
 <b71d1977-1a8b-4129-8c03-533031dbd004@gmail.com>
 <ZNmvegQirGI9ifVw@paquier.xyz>
 <2f84b397-d020-493c-924c-610b9dd18f8d@gmail.com>
 <ZNxrMdmgrlovw1qD@paquier.xyz>
 <f2204399-cfdb-40e7-9479-9373ec6ba0ab@gmail.com>
 <735fbd560ae914c96faaa23cc8d9a118@oss.nttdata.com>
 <d28ad85d-1deb-4648-92a0-f38273fc91ca@gmail.com>
User-Agent: Roundcube Webmail/1.4.11
Message-ID: <393fe340ab6980f399402a7dfbe13c4c@oss.nttdata.com>
Archived-At: 
 <https://www.postgresql.org/message-id/393fe340ab6980f399402a7dfbe13c4c%40oss.nttdata.com>
Precedence: bulk

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