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

Hi,

On Wed, Aug 27, 2025 at 02:13:39PM -0500, Sami Imseih wrote:
> Thanks for reviewing!
> 
> > === 1
> >
> > We need to check if tranche_name is NULL and report an error if that's the case.
> > If not, strlen() would segfault.
> 
> Added an error. Good call. The error message follows previously used
> convention.
> 
> ```
> +       if (!tranche_name)
> +               elog(ERROR, "tranche name cannot be null");
> ```

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

-       LWLockCounter = (int *) ((char *) MainLWLockArray - sizeof(int));
-       /* We use the ShmemLock spinlock to protect LWLockCounter */
-       SpinLockAcquire(ShmemLock);
-       result = (*LWLockCounter)++;
-       SpinLockRelease(ShmemLock);
+       if (!tranche_name)
+               elog(ERROR, "tranche name cannot be null");

The check has to be done before the strlen() call, if not it segfault:

Breakpoint 1, LWLockNewTrancheId (tranche_name=0x0) at lwlock.c:569
569             Size            tranche_name_length = strlen(tranche_name) + 1;
(gdb) n

Program received signal SIGSEGV, Segmentation fault.

> > === 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.
> 
> Done. I just mentioned NAMEDATALEN -1 in the docs.

Most of the places where NAMEDATALEN is mentioned in sgml files also mention
it's 64 bytes. Should we do the same here?

> > === 3
> >
> > 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?
> 
> OK, since that is the pattern used, I changed to strlcpy. But since we are doing
> checks in advance, I think it will be safe either way.

Agree.

- * stores the names of all dynamically-created tranches known to the current
- * process.  Any unused entries in the array will contain NULL.
+ * stores the names of all dynamically created tranches in shared memory.
+ * Any unused entries in the array will contain NULL. This variable is
+ * non-static so that postmaster.c can copy it to child processes in
+ * EXEC_BACKEND builds.

"Any unused entries in the array will contain NULL": this is not true anymore.
It now contains empty strings:

(gdb) p *(char(*)[64])NamedLWLockTrancheArray@4
$5 = {"tutu", '\000' <repeats 59 times>, '\000' <repeats 63 times>, '\000' <repeats 63 times>, '\000' <repeats 63
times>}

Regards,

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