Hi,
On 5/1/23 1:59 AM, Michael Paquier wrote:
On Fri, Apr 28, 2023 at 02:29:13PM +0200, Drouvot, Bertrand wrote:
On 4/27/23 8:13 AM, Michael Paquier wrote:
But do we need to merge more data than necessary? We could do things
in the simplest fashion possible while making the docs and code
user-friendly in the ordering: just add a section for Lock and LWLocks
in waiteventnames.txt with an extra comment in their headers and/or
data files to tell that waiteventnames.txt also needs a refresh.
Agree that it would fix the doc ordering and that we could do that.
Not much a fan of the part where a full paragraph of the SGML docs is
added to the .txt, particularly with the new handling for "Notes".
I understand your concern.
I'd rather shape the perl script to be minimalistic and simpler, even
if it means moving this paragraph about LWLocks after all the tables
are generated.
I'm not sure I like it. First, it does break the "Note" ordering as compare
to the current documentation. That's not a big deal though.
Secondly, what If we need to add some note(s) in the future for another wait
class? Having all the notes
after all the tables are generated would sound weird to me.
We could discuss another approach for the "Note" part if there is a need to add
one for an existing/new wait class
though.
Do we also need the comments in the generated header as well? My
initial impression was to just move these as comments of the .txt file
because that's where the new events would be added, as the .txt is the
main point of reference.
Oh I see. The advantage of the previous approach is to have them at both places
(.txt and header).
But that said I understand your point about having the perl script minimalistic
and simpler.
Please note that it creates 2 new "wait events":
WAIT_EVENT_EXTENSION and WAIT_EVENT_BUFFER_PIN.
Noted. Makes sense here.
Yup and that may help to add "custom" wait event for extensions too (need to
think about it once
this refactoring is done).
So, the change here..
+ # Exception here
+ if ($last =~ /^BufferPin/)
+ {
+ $last = "Buffer_Pin";
+ }
.. Implies the two following changes:
typedef enum
{
- WAIT_EVENT_BUFFER_PIN = PG_WAIT_BUFFER_PIN
+ WAIT_EVENT_BUFFER_PIN = PG_WAIT_BUFFERPIN
} WaitEventBufferPin;
[...]
static const char *
-pgstat_get_wait_buffer_pin(WaitEventBufferPin w)
+pgstat_get_wait_bufferpin(WaitEventBufferPin w)
I would be OK to remove this exception in the script as it does not
change anything for the end user (the wait event string is still
reported as "BufferPin"). This way, we keep things simpler in the
script.
Good point, agree.
This has as extra consequence to require a change in
wait_event.h so as PG_WAIT_BUFFER_PIN is renamed to PG_WAIT_BUFFERPIN,
equally fine by me. Logically, this rename should be done in a patch
of its own, for clarity.
Yes, I can look at it.
@@ -185,6 +193,7 @@ distprep:
$(MAKE) -C utils distprep
$(MAKE) -C utils/adt jsonpath_gram.c jsonpath_gram.h jsonpath_scan.c
$(MAKE) -C utils/misc guc-file.c
+ $(MAKE) -C utils/actvity wait_event_types.h pgstat_wait_event.c
Incorrect order, and incorrect name (s/actvity/activity/, lacking an
'i').
Nice catch.
+printf $h $header_comment, 'wait_event_types.h';
+printf $h "#ifndef WAITEVENTNAMES_H\n";
+printf $h "#define WAITEVENTNAMES_H\n\n";
Inconsistency detected here.
It seems to me that we'd better have a .gitignore in utils/activity/
for the new files.
Agree.
@@ -237,7 +237,7 @@ autoprewarm_main(Datum main_arg)
(void) WaitLatch(MyLatch,
WL_LATCH_SET | WL_EXIT_ON_PM_DEATH,
-1L,
- PG_WAIT_EXTENSION);
+ WAIT_EVENT_EXTENSION);
Perhaps this should also be part of a first, separate patch, with the
introduction of the new pgstat_get_wait_extension/bufferpin()
functions. Okay, it is not a big deal because the main patch
generates the enum for extensions which would be used here, but for
the sake of history clarity I'd rather refactor and rename all that
first.
Agree, I'll look at this.
The choices of LWLOCK and LOCK for the internal names was a bit
surprising, while we can be consistent with the rest and use "LWLock"
and "Lock".
Attached is a v7 with the portions I have adjusted, which is mostly OK
by me at this point. We are still away from the next CF, but I'll
look at that again when the v17 branch opens.
Thanks for the v7! I did not look at the details but just replied to this
thread.
I'll look at v7 when the v17 branch opens and propose the separate patch
mentioned above at that time too.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
