From: | Sami Imseih <samimseih(at)gmail(dot)com> |
---|---|
To: | Bertrand Drouvot <bertranddrouvot(dot)pg(at)gmail(dot)com> |
Cc: | pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Improve LWLock tranche name visibility across backends |
Date: | 2025-07-11 21:32:13 |
Message-ID: | CAA5RZ0vK_EuHwGj8nWkb1UwYjDbqvj+ZyiYPYn3CRsU8uwKUsQ@mail.gmail.com |
Views: | Whole Thread | Raw Message | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
> > and instead reuse the existing static hash table, which is
> > capped at 128 custom wait events:
> >
> > ```
> > #define WAIT_EVENT_CUSTOM_HASH_MAX_SIZE 128
> > ```
>
> That's probably still high enough, thoughts?
I have no reason to believe that this number could be too low.
I am not aware of an extension that will initialize more than a
couple of LWLocks.
> > or maybe we can just allow WaitEventCustomNew to take in the eventId, and
> > if it's > 0, then use the passed in value, otherwise generate the next eventId.
> >
> > I do like the latter approach more, what do you think?
>
> I think I do prefer it too, but in both cases we'll have to make sure there
> is no collision on the eventID (LWTRANCHE_FIRST_USER_DEFINED is currently
> 95).
As far as collisions are concerned, the key of the hash is the wait_event_info,
which is a bitwise OR of classId and eventId
```
wait_event_info = classId | eventId;
```
Do you think collision can still be possible?
Now, what I think will be a good API is to provide an alternative to
LWLockRegisterTranche,
which now takes in both a tranche ID and tranche_name. The new API I propose is
LWLockRegisterTrancheWaitEventCustom which takes only a tranche_name
and internally
calls WaitEventCustomNew to add a new wait_event_info to the hash
table. The wait_event_info
is made up of classId = PG_WAIT_LWLOCK and LWLockNewTrancheId().
I prefer we implement a new API for this to make it explicit that this
API will both register
a tranche and create a custom wait event. I do not think we should get
rid of LWLockRegisterTranche
because it is used by CreateLWLocks during startup and I don't see a
reason to change that.
See the attached v1.
What is missing from the attached v1 is:
1/ the documentation changes required in [0].
2/ in dsm_registry.c, we are going to have to modify GetNamedDSHash and
GetNamedDSA to use the new API. Here is an example of how that looks like
```
@@ -389,14 +385,12 @@ GetNamedDSHash(const char *name, const
dshash_parameters *params, bool *found)
entry->type = DSMR_ENTRY_TYPE_DSH;
/* Initialize the LWLock tranche for the DSA. */
- dsa_state->tranche = LWLockNewTrancheId();
sprintf(dsa_state->tranche_name, "%s%s", name,
DSMR_DSA_TRANCHE_SUFFIX);
- LWLockRegisterTranche(dsa_state->tranche,
dsa_state->tranche_name);
+ dsa_state->tranche =
LWLockRegisterTrancheWaitEventCustom(dsa_state->tranche_name);
```
Is there a concern with a custom wait event to be created implicitly
via the GetNamed* APIs?
3/ I still did not workout the max length requirement of the tranche
name, but I think
it would have to be as large as the GetNamed* APIs from the dsm
registry support.
I hope with v1, we can agree to the general direction of this change.
[0] https://www.postgresql.org/docs/current/xfunc-c.html
--
Sami
Attachment | Content-Type | Size |
---|---|---|
v1-0001-Create-LWLock-tranche-in-shared-memory.patch | application/octet-stream | 7.8 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Aleksander Alekseev | 2025-07-11 21:32:31 | Re: XLogCtl->ckptFullXid is unused |
Previous Message | Nathan Bossart | 2025-07-11 21:24:18 | Re: Improve LWLock tranche name visibility across backends |