Re: Improve LWLock tranche name visibility across backends
Sami Imseih <samimseih@gmail.com>
From: Sami Imseih <samimseih@gmail.com>
To: Nathan Bossart <nathandbossart@gmail.com>
Cc: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>,
Tom Lane <tgl@sss.pgh.pa.us>, Andres Freund <andres@anarazel.de>, Rahila Syed <rahilasyed90@gmail.com>, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2025-08-28T22:53:23Z
Lists: pgsql-hackers
Attachments
- v17-0001-dsm_registry-Use-one-LWLock-tranche-for-dshash-t.patch (application/octet-stream) patch v17-0001
- v17-0002-Move-dynamically-allocated-tranche-names-to-shar.patch (application/octet-stream) patch v17-0002
> * I modified the array of tranche names to be a char ** to simplify
> lookups. I also changed how it is first initialized in CreateLWLocks() a
> bit.
That works also.
> * I've left out the tests for now. Those are great for the development
> phase, but I'm not completely sold on committing them. In any case, I'd
> probably treat that as a follow-up effort once this stuff is committed.
I was mainly using them to verify the functionality since we lack tests in
this area. I think long-term, it will be good to have test coverage. We don't
need to decide now.
> I think this patch set will require reworking the "GetNamedLWLockTranche
> crashes on Windows in normal backend" patch [0], but AFAICT we can easily
> adjust it to scan through NamedLWLockTrancheNames instead.
Except, we will need to turn the char**NamedLWLockTranche into a struct
that will hold the num_lwlocks as well. Something like. But that is doable.
```
typedef struct NamedLWLockTranche
{
char trancheName[NAMEDATALEN];
int num_lwlocks;
} NamedLWLockTranche;
```
If there is no interest to backpatch [0], maybe we should just make this
change as part of this patch set. What do you think? I can make this change
in v18.
> Overall, I'm pretty happy about these patches. They simplify the API and
> the code while also fixing the problem with tranche name visibility.
Just a few things that were discussed earlier, that I incorporated now.
1/ We should be checking that tranche_name is NOT NULL when
LWLockNewTrancheId or RequestNamedLWLockTranche is called.
Also check for the length of the tranche name inside
RequestNamedLWLockTranche, instead of relying on an Assert to check
the length. I think that is much safer.
2/ Bertrand suggested to use strlcpy instead of strcpy when copying while
holding a spinlock [1]
3/ There was a doc update to mention the 63 byte tranche length. We can
skip that for now, since the ERROR message in v16 is clear about the
limit.
[0] https://postgr.es/m/CAA5RZ0v1_15QPg5Sqd2Qz5rh_qcsyCeHHmRDY89xVHcy2yt5BQ%40mail.gmail.com
[1] https://www.postgresql.org/message-id/CAA5RZ0ve-fHuNYW-ruMwg1y1v7-aCqMm_MiNq1KOdg2Y2-pKDw%40mail.gmail.com
--
Sami
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