Re: Improve LWLock tranche name visibility across backends - Mailing list pgsql-hackers

Hi,

On Tue, Aug 26, 2025 at 05:50:34PM -0500, Sami Imseih wrote:
> fixed the issues mentioned above in v13.
> 
> > We probably need to do the sprintf/strcpy before LWLockNewTrancheId().
> > Also, I'm thinking we should just use the same tranche for both the DSA and
> > the dshash table [0] to evade the DSMR_DSA_TRANCHE_SUFFIX problem, i.e.,
> > those tranche names potentially require more space.
> 
> v13 also includes a separate patch for this. It removes the
> DSMR_DSA_TRANCHE_SUFFIX, and dsh and dsa now use the same
> user defined tranche. The tranche name is also limited to
> the max length of a named tranche, MAX_NAMED_TRANCHES_NAME_LEN,
> coming from lwlock.h

Thanks for the new version.

=== 1

+LWLockNewTrancheId(const char *tranche_name)
 {
-       int                     result;
-       int                *LWLockCounter;
+       int                     tranche_id,
+                               index,
+                          *LWLockCounter;
+       Size            tranche_name_length = strlen(tranche_name) + 1;

We need to check if tranche_name is NULL and report an error if that's the case.
If not, strlen() would segfault.

=== 2

+       if (tranche_name_length > MAX_NAMED_TRANCHES_NAME_LEN)
+               elog(ERROR, "tranche name too long");

I think that we should mention in the doc that the tranche name is limited to
63 bytes.

=== 3


-       /* Convert to array index. */
-       tranche_id -= LWTRANCHE_FIRST_USER_DEFINED;
+       tranche_id = (*LWLockCounter)++;
+       index = tranche_id - LWTRANCHE_FIRST_USER_DEFINED;

-       /* If necessary, create or enlarge array. */
-       if (tranche_id >= LWLockTrancheNamesAllocated)
+       if (index >= MAX_NAMED_TRANCHES)
        {
-               int                     newalloc;
+               SpinLockRelease(ShmemLock);
+               elog(ERROR, "too many LWLock tranches registered");
+       }

-               newalloc = pg_nextpower2_32(Max(8, tranche_id + 1));
+       /* Directly copy into the NamedLWLockTrancheArray */
+       strcpy((char *) NamedLWLockTrancheArray + (index * MAX_NAMED_TRANCHES_NAME_LEN),
+                  tranche_name);

I was skeptical about using strcpy() while we hold a spinlock. I do see some
examples with strlcpy() though (walreceiver.c for example), so that looks OK-ish.

Using strcpy() might be OK too, as we already have validated the length, but maybe
it would be safer to switch to strlcpy(), instead?

Regards,

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