From: | Sami Imseih <samimseih(at)gmail(dot)com> |
---|---|
To: | Dagfinn Ilmari Mannsåker <ilmari(at)ilmari(dot)org> |
Cc: | Nathan Bossart <nathandbossart(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: add function for creating/attaching hash table in DSM registry |
Date: | 2025-06-05 18:38:25 |
Message-ID: | CAA5RZ0v4c6p7VcvatRLyHpWK7fzSmUU7LsdYAL31kedBqTVjfg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Thanks for this patch! I have implemented this pattern several times,
so this is really helpful.
I have a few early comments, but I plan on trying this out next.
1/
> > +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?
NamedLWLockTrancheRequest uses NAMEDATALEN = 64 bytes for the
tranche_name
typedef struct NamedLWLockTrancheRequest
{
char tranche_name[NAMEDATALEN];
int num_lwlocks;
} NamedLWLockTrancheRequest;
but in the case here, "_dsa" or "_dsh" will occupy another 4 bytes.
I think instead of hardcoding, we should #define a length,
i.e. #define NAMEDDSMTRANCHELEN (NAMEDATALEN + 4)
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);
3/ It will be good to "Assert(dsh)" before "return dsh;" for safety?
MemoryContextSwitchTo(oldcontext);
LWLockRelease(DSMRegistryLock);
return dsh;
}
--
Sami Imseih
Amazon Web Services (AWS)
From | Date | Subject | |
---|---|---|---|
Next Message | Alexander Lakhin | 2025-06-05 19:00:00 | Re: Non-reproducible AIO failure |
Previous Message | Andres Freund | 2025-06-05 18:32:10 | Re: postmaster uses more CPU in 18 beta1 with io_method=io_uring |