Re: Improve LWLock tranche name visibility across backends

From: Sami Imseih <samimseih(at)gmail(dot)com>
To: Nathan Bossart <nathandbossart(at)gmail(dot)com>
Cc: Bertrand Drouvot <bertranddrouvot(dot)pg(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Andres Freund <andres(at)anarazel(dot)de>, Rahila Syed <rahilasyed90(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Improve LWLock tranche name visibility across backends
Date: 2025-08-28 22:53:23
Message-ID: CAA5RZ0vw-r1qjHw5hUNTzAyCiK3KQ-r+G5ECfWSn+qjdu-fpnw@mail.gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> * 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

Attachment Content-Type Size
v17-0001-dsm_registry-Use-one-LWLock-tranche-for-dshash-t.patch application/octet-stream 5.9 KB
v17-0002-Move-dynamically-allocated-tranche-names-to-shar.patch application/octet-stream 23.5 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tomas Vondra 2025-08-28 23:00:58 Re: index prefetching
Previous Message Matheus Alcantara 2025-08-28 22:31:35 Re: LISTEN/NOTIFY bug: VACUUM sets frozenxid past a xid in async queue