Re: Autogenerate some wait events code and documentation

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