Re: Improve LWLock tranche name visibility across backends
Nathan Bossart <nathandbossart@gmail.com>
From: Nathan Bossart <nathandbossart@gmail.com>
To: Rahila Syed <rahilasyed90@gmail.com>
Cc: Sami Imseih <samimseih@gmail.com>, Bertrand Drouvot <bertranddrouvot.pg@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>, Andres Freund <andres@anarazel.de>, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2025-09-02T15:38:23Z
Lists: pgsql-hackers
Attachments
On Mon, Sep 01, 2025 at 06:35:55PM +0530, Rahila Syed wrote:
> Please see below for some comments regarding v20 patch.
Thanks for looking.
> 1. Following unused declaration is present in the lwlock.h file.
>
> extern void LWLockRegisterTranche(int tranche_id, const char *tranche_name);
Whoops, good catch.
> If the intention is to delay acquiring the spin lock when trancheId is
> less than LocalLWLockCounter, it seems possible that we might read
> NamedLWLockTrancheNames in the GetLWTrancheName function after
> LocalLWLockCounter has been updated, but before NamedLWLockTrancheNames
> has been updated in the LWLockNewTrancheId function. To prevent reading
> an outdated tranche name, we should acquire the spin lock unconditionally
> in the GetLWTrancheName function
We only ever add new tranche names to the end of the list, so we should be
able to avoid the lock for the vast majority of tranche name lookups. We
hold the lock while we increment LWLockCounter and add a name, and also
whenever we update LocalLWLockCounter. This should prevent any unsafe
accesses to NamedLWLockTrancheNames.
I've added some additional commentary about this in v21.
> + ereport(ERROR,
> + (errmsg("maximum number of tranches already registered"),
> + errdetail("No more than %d tranches may be registered.",
> + MAX_NAMED_TRANCHES)));
> + }
>
> Since this patch sets a maximum limit on the number of LW lock tranches
> that can be registered, would it make sense to offer a configurable
> option rather than using a hard-coded MAX_NUM_TRANCHES? This will allow
> users who have reached the maximum limit to register their LW Locks.
Hm. I'm not sure. I think it would be good to offer a way to accommodate
more tranche names that didn't involve recompiling, but I don't know
whether that situation is likely enough in practice to add yet another GUC.
Thus far, we've been operating under the assumption that we'll be able to
choose a number far beyond any realistic usage. If others have opinions
about this, please share...
--
nathan
Commits
-
test_dsa: Avoid leaking LWLock tranches.
- c5c74282f2ea 19 (unreleased) landed
-
Teach DSM registry to ERROR if attaching to an uninitialized entry.
- b26d76f64327 18.2 landed
- ac2800ddc185 17.8 landed
- 1165a933aab1 19 (unreleased) landed
-
Add a test harness for the LWLock tranche code.
- 16607718c010 19 (unreleased) landed
-
Revert recent change to RequestNamedLWLockTranche().
- d814d7fc3d52 19 (unreleased) landed
-
Move dynamically-allocated LWLock tranche names to shared memory.
- 38b602b0289f 19 (unreleased) landed
-
Prepare DSM registry for upcoming changes to LWLock tranche names.
- 5487058b56e0 19 (unreleased) landed
-
Add GetNamedDSA() and GetNamedDSHash().
- fe07100e82b0 19 (unreleased) cited