Verify predefined LWLocks tranches have entries in wait_event_names.txt

On Fri, Jul 18, 2025 at 01:39:15PM +0000, Bertrand Drouvot wrote:

0002 - While doing changes in generate-lwlocknames.pl, I noticed that
$continue is not used (Oversight in commit da952b415f4), so let's remove it.

I haven't looked closely at the other patches, but I went ahead and
committed this one to get it out of the way.

--
nathan

On Fri, Jul 18, 2025 at 01:39:15PM +0000, Bertrand Drouvot wrote:

+#define PG_BUILTIN_LWTRANCHE(id, name) [id] = name,
+#include "storage/lwlocktranchelist.h"
+#undef PG_BUILTIN_LWTRANCHE

Why not reuse PG_LWLOCK for this?

+	# Stop recording if we reach another section.
+	last if /^Section:/;

Can we add a note to wait_event_names.txt about the required
ordering/matching of the non-predefined LWLocks? Otherwise, these patches
look pretty good to me.

--
nathan

On Mon, Jul 21, 2025 at 03:20:55PM -0500, Nathan Bossart wrote:

On Fri, Jul 18, 2025 at 01:39:15PM +0000, Bertrand Drouvot wrote:

+#define PG_BUILTIN_LWTRANCHE(id, name) [id] = name,
+#include "storage/lwlocktranchelist.h"
+#undef PG_BUILTIN_LWTRANCHE

Why not reuse PG_LWLOCK for this?

+	# Stop recording if we reach another section.
+	last if /^Section:/;

Can we add a note to wait_event_names.txt about the required
ordering/matching of the non-predefined LWLocks? Otherwise, these patches
look pretty good to me.

Something else I just thought of: could we remove the list of built-in
tranches in lwlock.h with some macro magic that generates it from
lwlocktranchelist.h, too?

--
nathan

On Mon, Jul 21, 2025 at 03:28:14PM -0500, Nathan Bossart wrote:

On Mon, Jul 21, 2025 at 03:20:55PM -0500, Nathan Bossart wrote:

Can we add a note to wait_event_names.txt about the required
ordering/matching of the non-predefined LWLocks? Otherwise, these patches
look pretty good to me.

Something else I just thought of: could we remove the list of built-in
tranches in lwlock.h with some macro magic that generates it from
lwlocktranchelist.h, too?

Ah, you mean removing the need to have to maintain BuiltinTrancheIds.
This structure depends on NUM_INDIVIDUAL_LWLOCKS for the start value.
Not really an objection per-se, but trying to automate everything may
impact the readability of this area of the code.
--
Michael

On Tue, Jul 22, 2025 at 08:02:52AM +0900, Michael Paquier wrote:

Ah, you mean removing the need to have to maintain BuiltinTrancheIds.
This structure depends on NUM_INDIVIDUAL_LWLOCKS for the start value.
Not really an objection per-se, but trying to automate everything may
impact the readability of this area of the code.

I bet we could maintain a decent level of readability with some extra
commentary. IMHO it's worth it to avoid maintaining duplicate lists. But
that's not something I feel terribly strong about, if others disagree.
FWIW I was imagining something like this:

typedef enum BuiltinTrancheIds
{
LWTRANCHE_INVALID = NUM_INDIVIDUAL_LWLOCKS - 1,
#define PG_BUILTIN_LWTRANCHE(id, name) id,
#include "storage/lwlocktranchelist.h"
#undef PG_BUILTIN_LWTRANCHE
LWTRANCHE_FIRST_USER_DEFINED,
} BuiltinTrancheIds;

--
nathan

On Mon, Jul 21, 2025 at 08:34:41PM -0500, Nathan Bossart wrote:

I bet we could maintain a decent level of readability with some extra
commentary. IMHO it's worth it to avoid maintaining duplicate lists. But
that's not something I feel terribly strong about, if others disagree.
FWIW I was imagining something like this:

typedef enum BuiltinTrancheIds
{
LWTRANCHE_INVALID = NUM_INDIVIDUAL_LWLOCKS - 1,

Something like that would be OK for me.
--
Michael

Hi,

On Mon, Jul 21, 2025 at 03:28:14PM -0500, Nathan Bossart wrote:

On Mon, Jul 21, 2025 at 03:20:55PM -0500, Nathan Bossart wrote:

On Fri, Jul 18, 2025 at 01:39:15PM +0000, Bertrand Drouvot wrote:

+#define PG_BUILTIN_LWTRANCHE(id, name) [id] = name,
+#include "storage/lwlocktranchelist.h"
+#undef PG_BUILTIN_LWTRANCHE

Why not reuse PG_LWLOCK for this?

+	# Stop recording if we reach another section.
+	last if /^Section:/;

Can we add a note to wait_event_names.txt about the required
ordering/matching of the non-predefined LWLocks? Otherwise, these patches
look pretty good to me.

Something else I just thought of: could we remove the list of built-in
tranches in lwlock.h with some macro magic that generates it from
lwlocktranchelist.h, too?

Thanks for looking at the patch series!

Yeah, I also had in mind to auto-generate this enum list (see last sentence
in the initial email [1]/messages/by-id/aHpOgwuFQfcFMZ/B@ip-10-97-1-34.eu-west-3.compute.internal) but did not think that much on how to do it and was
waiting for this patch series to go in before looking at it.

But as I like your macro idea and as you mentioned it here too, then let's do
it now: it's done in 0003 attached.

[1]: /messages/by-id/aHpOgwuFQfcFMZ/B@ip-10-97-1-34.eu-west-3.compute.internal

Regards,

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

Hi,

On Mon, Jul 21, 2025 at 03:20:55PM -0500, Nathan Bossart wrote:

On Fri, Jul 18, 2025 at 01:39:15PM +0000, Bertrand Drouvot wrote:

+#define PG_BUILTIN_LWTRANCHE(id, name) [id] = name,
+#include "storage/lwlocktranchelist.h"
+#undef PG_BUILTIN_LWTRANCHE

Why not reuse PG_LWLOCK for this?

I was not sure we'd want add a new file or add those in lwlocklist.h (in which
case 2 macros would have been simpler). As it looks like we are going with a
new file then yeah we can reuse PG_LWLOCK: done in v2 just shared up-thread.

+	# Stop recording if we reach another section.
+	last if /^Section:/;

Can we add a note to wait_event_names.txt about the required
ordering/matching of the non-predefined LWLocks?

Sure, done in v2 too.

Regards,

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

On Tue, Jul 22, 2025 at 06:23:50AM +0000, Bertrand Drouvot wrote:

Sure, done in v2 too.

I stared at this patch some more and came up with the attached. The
biggest change is that I've moved the list of built-in LWLock tranches to
the existing lwlocklist.h file. That simplifies the patch and centralizes
these lists. This is arguably a bit too much preprocessor magic, though.
Thoughts?

--
nathan

On Tue, Jul 22, 2025 at 02:25:13PM -0500, Nathan Bossart wrote:

I stared at this patch some more and came up with the attached. The
biggest change is that I've moved the list of built-in LWLock tranches to
the existing lwlocklist.h file. That simplifies the patch and centralizes
these lists. This is arguably a bit too much preprocessor magic, though.
Thoughts?

With the argument about checking the consistency of the data between
wait_event_names.txt, that looks like an improvement to me. And as
far as I can see, the check you are adding in generate-lwlocknames.pl
also makes sure that the ordering of the entries is correct.

In short, I side with the argument that this extra magic will save
cycles overall.
--
Michael

Hi,

On Wed, Jul 23, 2025 at 12:23:37PM +0900, Michael Paquier wrote:

On Tue, Jul 22, 2025 at 02:25:13PM -0500, Nathan Bossart wrote:

I stared at this patch some more and came up with the attached. The
biggest change is that I've moved the list of built-in LWLock tranches to
the existing lwlocklist.h file. That simplifies the patch and centralizes
these lists. This is arguably a bit too much preprocessor magic, though.

Yeah, but OTOH we avoid the addition of a new file and we avoid changing some
meson and make files. Also the distinction is still clear as we are using a new
macro name. So that's fine by me.

Thoughts?

I noticed that you removed the LWTRANCHE_ prefix in lwlocklist.h, while I agree
that's not needed, that could be misleading for people that grep for things like
"LWTRANCHE_XACT_BUFFER" for example. I don't think that's a big deal though.

Also, I think we don't need to read lwlocklist.h twice in generate-lwlocknames.pl.

PFA v4 where the only change compared to v3 is that it reads lwlocklist.h once
in generate-lwlocknames.pl.

Regards,

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

On Wed, Jul 23, 2025 at 06:22:16AM +0000, Bertrand Drouvot wrote:

PFA v4 where the only change compared to v3 is that it reads lwlocklist.h once
in generate-lwlocknames.pl.

Committed.

--
nathan