From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: Sami Imseih <samimseih@gmail.com>
Cc: Nathan Bossart <nathandbossart@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Andres Freund <andres@anarazel.de>
Cc: Rahila Syed <rahilasyed90@gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Improve LWLock tranche name visibility across backends
Date: Wed, 27 Aug 2025 09:28:40 +0000
Message-ID: <aK7PyCbPib+/JO2+@ip-10-97-1-34.eu-west-3.compute.internal> (raw)
In-Reply-To: <CAA5RZ0v=vvzn4mvejQVA1iFabxSTUQZPDuTCAF0WqA41LKns7g@mail.gmail.com>
References: <CAA5RZ0tKoQeTuRsFmUjPAOEXBiyEGyePsqCAy-_oX4eFHSCmHw@mail.gmail.com>
<aKzP-H-KzPay-ByM@nathan>
<CAA5RZ0vh7jrGDwJgkKtw-LR5fBH-tRNpGJnPivdKedtqJ7HGCQ@mail.gmail.com>
<aKzZV8Acib6pFesE@nathan>
<CAA5RZ0vWh2uxG99iUb3P4nsoDLk0x7Yuai83AbbGs3YZvJM8QQ@mail.gmail.com>
<aKz8cHo2oBr-6uq5@nathan>
<CAA5RZ0uYpXw8yTpCPiDAvwt9WtXesKM_vebbCjBm4MQBVnchMw@mail.gmail.com>
<CAA5RZ0u=Fb8dDdPrEzawYjVqAmYB_PTP4Yy3tV78=_gcvXrjjA@mail.gmail.com>
<aK4Vr0puSQqPI5vS@nathan>
<CAA5RZ0v=vvzn4mvejQVA1iFabxSTUQZPDuTCAF0WqA41LKns7g@mail.gmail.com>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: bertranddrouvot.pg@gmail.com, samimseih@gmail.com, nathandbossart@gmail.com, tgl@sss.pgh.pa.us, andres@anarazel.de, rahilasyed90@gmail.com
Subject: Re: Improve LWLock tranche name visibility across backends
In-Reply-To: <aK7PyCbPib+/JO2+@ip-10-97-1-34.eu-west-3.compute.internal>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox