Re: Improve LWLock tranche name visibility across backends

Hi,

On Mon, Aug 04, 2025 at 10:47:45PM -0500, Sami Imseih wrote:
> > > > With a local hash table, I don't think it's necessary to introduce new
> > > > code for managing
> > > > a DSA based list of tranche names as is done in v3. We can go back to
> > > > storing the shared
> > > > trance names in dshash.
> > > >
> > > > What do you think?
> > >
> > > My first thought is that a per-backend hash table seems too
> > > expensive/complicated for this.  Couldn't it just be an array like we have
> > > now?
> >
> > We can, but I was considering simplicity of implementation, and using a
> > local hash table is slightly simpler.
> >
> > That said, since we're dealing with an append-only data structure, a hash
> > table is probably more than we need. All we need is index-based lookup,
> > so I’ll go with the local array to mirror the shared ( dsa ) array.

+1 for the local array.

> Here is v4.

Thanks for the patch update!

> We can keep using the local array as the backend local array.
> It will already include the tranches registered during postmaster startup,
> and it will be updated during tranche name lookup from the dsa based
> array.

> I added a new routine UpdateLocalTrancheName which can be used
> in both LWLockRegisterTranche and GetLWTrancheName to append to the
> local tranche during both postmaster startup and afterwards.

I did not look at the code in details, just played a bit with it and found
some issues.

Issue 1 --

If I register enough tranches to go to:

+               /* Resize if needed */
+               if (LWLockTrancheNames.shmem->count >= LWLockTrancheNames.shmem->allocated)
+               {
+                       newalloc = pg_nextpower2_32(Max(LWLOCK_TRANCHE_NAMES_INIT_SIZE, LWLockTrancheNames.shmem->allocated + 1));
+                       new_list_ptr = dsa_allocate(LWLockTrancheNames.dsa, newalloc * sizeof(dsa_pointer));
+                       new_name_ptrs = dsa_get_address(LWLockTrancheNames.dsa, new_list_ptr);
+                       memcpy(new_name_ptrs, current_ptrs, LWLockTrancheNames.shmem->allocated * sizeof(dsa_pointer));
+                       dsa_free(LWLockTrancheNames.dsa, *current_ptrs);

then I get:

ERROR:  dsa_area could not attach to a segment that has been freed

I think we should

"
dsa_free(LWLockTrancheNames.dsa, LWLockTrancheNames.shmem->list_ptr)
"

instead.

Issue 2 --

If an extension calls RequestNamedLWLockTranche() it will register the same
tranche twice:

(gdb) p LWLockTrancheNames.local[0]
$1 = 0x7acf5a40c420 "pg_playmem"
(gdb) p LWLockTrancheNames.local[97]
$2 = 0x7acf5a40c420 "pg_playmem"

First (local[0]) during LWLockNewTrancheId() during InitializeLWLocks()
Second (local[97]) during LWLockRegisterTranche() during CreateLWLocks()

Maybe we should put this back?

-       /* This should only be called for user-defined tranches. */
-       if (tranche_id < LWTRANCHE_FIRST_USER_DEFINED)
-               return;

-       /* Convert to array index. */
-       tranche_id -= LWTRANCHE_FIRST_USER_DEFINED;

and remove:

+       tranche_index = tranche_id - LWTRANCHE_FIRST_USER_DEFINED;

from LWLockNewTrancheId()?

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com