Re: Improve LWLock tranche name visibility across backends

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(at)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