MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit
Date: Thu, 17 Aug 2023 10:53:02 +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: <f2204399-cfdb-40e7-9479-9373ec6ba0ab@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>
User-Agent: Roundcube Webmail/1.4.11
Message-ID: <735fbd560ae914c96faaa23cc8d9a118@oss.nttdata.com>
Archived-At: 
 <https://www.postgresql.org/message-id/735fbd560ae914c96faaa23cc8d9a118%40oss.nttdata.com>
Precedence: bulk

Hi,

Thank you for creating the patch!
I think it is a very useful view as a user.

I will share some thoughts about the v6 patch.

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

* The HTML tag match is not shortest match.
-frozenid Waiting to update pg_database.datfrozenxid and 
pg_database.datminmxid
+frozenid Waiting to update datminmxid

* There are two blanks before "about". 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

* 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

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.

3)

Would index == els be better for the following Assert?
+	Assert(index <= els);

4)

There is a typo. alll -> all
+	/* Allocate enough space for alll entries */

5)

BTW, although I think this is outside the scope of this patch,
it might be a good idea to be able to add a description to the
API for custom wait events.

Regards,
-- 
Masahiro Ikeda
NTT DATA CORPORATION