Message-ID: <d28ad85d-1deb-4648-92a0-f38273fc91ca@gmail.com>
Date: Thu, 17 Aug 2023 07:53:02 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: WIP: new system catalog pg_wait_event
Content-Language: en-US
To: Masahiro Ikeda <ikedamsh@oss.nttdata.com>
Cc: Michael Paquier <michael@paquier.xyz>,
 Kyotaro Horiguchi <horikyota.ntt@gmail.com>, tgl@sss.pgh.pa.us,
 pgsql-hackers@lists.postgresql.org
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>
From: "Drouvot, Bertrand" <bertranddrouvot.pg@gmail.com>
In-Reply-To: <735fbd560ae914c96faaa23cc8d9a118@oss.nttdata.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/d28ad85d-1deb-4648-92a0-f38273fc91ca%40gmail.com>
Precedence: bulk

Hi,

On 8/17/23 3:53 AM, Masahiro Ikeda wrote:
> 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.
> 

Thanks for looking at it!

> 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.

> * 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.

> 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.

> * 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.

> 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.

> 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! Fixed in v7.

[1]: https://www.postgresql.org/message-id/dd836027-2e9e-4df9-9fd9-7527cd1757e1%40gmail.com

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com