Date: Tue, 12 Aug 2025 14:48:50 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: Sami Imseih <samimseih@gmail.com>
Cc: Rahila Syed <rahilasyed90@gmail.com>,
	Bertrand Drouvot <bertranddrouvot.pg@gmail.com>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Improve LWLock tranche name visibility across backends
Message-ID: <aJuaoouYv8l8f0PS@nathan>
References: 
 <CAA5RZ0skN3ATpEvcFCUCiHDqvs6N95uzAo74wUSR93Bdgrq5Xg@mail.gmail.com>
 <CAA5RZ0uYVpKzQbP6JJhGv=JBGfOMVwzTtyd789P_hJxXXyKuhw@mail.gmail.com>
 <aJHSULQPfY9qUTFY@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAA5RZ0tU9j-jEPUCS0=K6PCzYY=291O9Uky9ndbqH_BDp4Hc=Q@mail.gmail.com>
 <CAA5RZ0txvpg7pZGRt2aUmBck8rVK1nUZkwpu=ehyL6kmHd_apQ@mail.gmail.com>
 <aJMUvF0Pbswf4oUX@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAH2L28uCaeuyz9iZOfHK_XPaJw06whE+92g0pGW6PytV1YHSbg@mail.gmail.com>
 <CAA5RZ0uh_WviMnh9z-oZP32SSE5MBEshzx5Cfe0rFYnPwQNzJw@mail.gmail.com>
 <CAA5RZ0tQ+EYSTOd2hQ8RXdsNfGBLAtOe-YmnsTE6ZVg0E-4qew@mail.gmail.com>
 <CAA5RZ0siw2MrVjf9iPYRpe8sgZW+TJRqmxKfCr=QS9mX+ZF8Lw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAA5RZ0siw2MrVjf9iPYRpe8sgZW+TJRqmxKfCr=QS9mX+ZF8Lw@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/aJuaoouYv8l8f0PS%40nathan>
Precedence: bulk

I haven't followed the latest discussion, but I took a look at the patch.

+      It is possible to use a <literal>tranche_id</literal> that was not retrieved
+      using <function>LWLockNewTrancheId</function>, but this is not recommended.
+      The ID may clash with an already registered tranche name, or the specified
+      name may not be found. In such cases, looking up the name will return a generic
+      "extension" tranche name.

Is there any reason to continue allowing this?  For example, maybe we could
ERROR if LWLockInitialize()/GetLWTrancheName() are given a tranche_id
greater than the number allocated.  I guess I'm not following why we should
gracefully handle these kinds of coding errors, especially when they result
in unhelpful behavior like an "extension" tranche.

+#else
+elog(ERROR, "injection points not supported");
+#endif

This causes compilation to fail when injection points are not enabled.

I haven't combed through the patch character-by-character, but upon a
read-through, the general shape looks reasonable to me.  As a general note,
I'd suggest adding more commentary throughout and finding opportunities to
simplify and/or clean up the code as much as possible.

-- 
nathan