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
