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
