Re: add function for creating/attaching hash table in DSM registry

On Thu, Jun 05, 2025 at 01:38:25PM -0500, Sami Imseih wrote:
> I have a few early comments, but I plan on trying this out next.

Thanks for reviewing.

>> > +typedef struct NamedDSMHashState
>> > +{
>> > + dsa_handle dsah;
>> > + dshash_table_handle dshh;
>> > + int dsa_tranche;
>> > + char dsa_tranche_name[68]; /* name + "_dsa" */
>> > + int dsh_tranche;
>> > + char dsh_tranche_name[68]; /* name + "_dsh" */
>> > +} NamedDSMHashState;
>>
>> I don't have enough knowledge to review the rest of the patch, but
>> shouldn't this use NAMEDATALEN, rather than hard-coding the default
>> length?

I straightened this out in v2. I've resisted using NAMEDATALEN because
this stuff is unrelated to the name type. But I have moved all the lengths
and suffixes to macros.

> NamedLWLockTrancheRequest uses NAMEDATALEN = 64 bytes for the
> tranche_name
>
> typedef struct NamedLWLockTrancheRequest
> {
> char tranche_name[NAMEDATALEN];
> int num_lwlocks;
> } NamedLWLockTrancheRequest;

I think the NAMEDATALEN limit only applies to tranches requested at startup
time. LWLockRegisterTranche() just saves whatever pointer you give it, so
AFAICT there's no real limit there.

> 2/ Can you group the dsa and dsh separately. I felt this was a bit
> difficult to read?
>
> + /* Initialize LWLock tranches for the DSA and dshash table. */
> + state->dsa_tranche = LWLockNewTrancheId();
> + state->dsh_tranche = LWLockNewTrancheId();
> + sprintf(state->dsa_tranche_name, "%s_dsa", name);
> + sprintf(state->dsh_tranche_name, "%s_dsh", name);
> + LWLockRegisterTranche(state->dsa_tranche,
> state->dsa_tranche_name);
> + LWLockRegisterTranche(state->dsh_tranche,
> state->dsh_tranche_name);

Done.

> 3/ It will be good to "Assert(dsh)" before "return dsh;" for safety?
>
> MemoryContextSwitchTo(oldcontext);
> LWLockRelease(DSMRegistryLock);
>
> return dsh;

Eh, I would expect the tests to start failing horribly if I managed to mess
that up.

--
nathan