Message-ID: <e8a1e3c2-26e5-8a7a-f821-eee92ab0c8e2@gmail.com>
Date: Wed, 26 Apr 2023 18:51:46 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.10.1
Subject: Re: Autogenerate some wait events code and documentation
To: Michael Paquier <michael@paquier.xyz>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: <77a86b3a-c4a8-5f5d-69b9-d70bbf2e9b98@gmail.com>
 <6b6cdc76-e10e-95f8-62cf-3a3137c86df5@gmail.com>
 <ZECQvY2tt6p+ZHiX@paquier.xyz>
 <2e649ed7-6c66-5d6a-16d4-5d5367d40e69@gmail.com>
 <ZEX0QT6vROUONVRH@paquier.xyz>
 <ff27ae4a-edaa-7766-0b43-c3a7b14fca84@gmail.com>
 <ZEdh2+46GuhCqW6b@paquier.xyz>
Content-Language: en-US
From: "Drouvot, Bertrand" <bertranddrouvot.pg@gmail.com>
In-Reply-To: <ZEdh2+46GuhCqW6b@paquier.xyz>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/e8a1e3c2-26e5-8a7a-f821-eee92ab0c8e2%40gmail.com>
Precedence: bulk

Hi,

On 4/25/23 7:15 AM, Michael Paquier wrote:
> On Mon, Apr 24, 2023 at 09:03:53AM +0200, Drouvot, Bertrand wrote:
>> Oh right, fixed.
> 
> I may tweak a few things if I put my hands on it, but that looks
> pretty solid seen from here.. 

Glad to hear! ;-)

> I have spotted a few extra issues.
> 
> One thing I have noticed with v4 is that the order of the tables
> generated in wait_event_types.h and the SGML docs is inconsistent with
> previous versions, and these are not in an alphabetical order.  HEAD
> orders them as Activity, BufferPin, Client, Extension, IO, IPC, Lock,
> LWLock and Timeout.  This patch switches the order to become
> different, and note that the first table describing each of the wait
> event type classes gets it right.
> 

Right, ordering being somehow broken is also something I did mention initially when I first
presented this patch up-thread. That's also due to the fact that this patch
does not autogenerate PG_WAIT_LWLOCK, PG_WAIT_LOCK, PG_WAIT_BUFFER_PIN and PG_WAIT_EXTENSION.

> It seems to me that you should apply an extra ordering in
> generate-waiteventnames.pl to make sure that the tables are printed in
> the same order as previously, around here:
> +# Generate the output files
> +foreach $waitclass (keys %hashwe) {
> 

Yeah but that would still affect only the auto-generated one and then
result to having the wait event part of the documentation "monitoring-stats"
not ordered as compared to the "Wait Event Types" Table.

And as we have only one "include" in doc/src/sgml/monitoring.sgml for all the
auto-generated one, the ordering would still be broken.

> (The table describing all the wait event types could be removed from
> the SGML docs as well, at the cost of having their description in the
> new .txt file.  However, as these are long, it would make the .txt
> file much messier, so not doing this extra part is OK for me.)

Right, but that might be a valuable option to also fix the ordering issue
mentioned above (need to look deeper at this).

>
> - * Use this category when a process is waiting because it has no work to do,
> - * unless the "Client" or "Timeout" category describes the situation better.
> - * Typically, this should only be used for background processes
> 
> wait_event.h includes a set of comments describing each category, that
> this patch removes.  Rather than removing this information, which is
> helpful to have around, why not making them comments of
> waiteventnames.txt instead?  Losing this information would be sad.
>

Yeah, good point, I'll look at this.

> +#   src/backend/utils/activity/pgstat_wait_event.c
> +#      c functions to get the wait event name based on the enum
> Nit.  'c' should be upper-case.
> 
>      }
> +
>      if (IsNewer(
>              'src/include/storage/lwlocknames.h',
> Not wrong, but this is an unrelated diff.
> 

Yeah, probably due to a pgindent run.

> +if %DIST%==1 if exist src\backend\utils\activity\pgstat_wait_event.c del /q src\backend\utils\activity\pgstat_wait_event.c
>   if %DIST%==1 if exist src\backend\storage\lmgr\lwlocknames.h del /q src\backend\storage\lmgr\lwlocknames.h
> +if %DIST%==1 if exist src\backend\utils\activity\wait_event_types.h del /q src\backend\utils\activity\wait_event_types.h
> The order here is off a bit.  Missed that previously..
> 
> perltidy had better be run on generate-waiteventnames.pl (I can do
> that myself, no worries), as a couple of lines' format don't seem
> quite in line.

Will do, no problem at all.

Regards,

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