Re: Shared memory size computation oversight?

On Thu, Mar 04, 2021 at 08:43:51AM +0000, Georgios wrote:
>
> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> On Thursday, March 4, 2021 9:21 AM, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> > On Thu, Mar 04, 2021 at 03:23:33PM +0800, Julien Rouhaud wrote:
> >
> > > I was also considering adding new (add|mull)_*_size functions to avoid having
> > > too messy code. I'm not terribly happy with xxx_shm_size(), as not all call to
> > > those functions would require an alignment. Maybe (add|mull)shmemalign_size?
> > > But before modifying dozens of calls, should we really fix those or only
> > > increase a bit the "slop factor", or a mix of it?
> > > For instance, I can see that for instance BackendStatusShmemSize() never had
> > > any padding consideration, while others do.
> > > Maybe only fixing contribs, some macro like PredXactListDataSize that already
> > > do a MAXALIGN, SimpleLruShmemSize and hash_estimate_size() would be a short
> > > patch and should significantly improve the estimation.
> >
> > The lack of complaints in this area looks to me like a sign that we
> > may not really need to backpatch something, so I would not be against
> > a precise chirurgy, with a separate set of {add,mul}_size() routines
> > that are used where adapted, so as it is easy to track down which size
> > estimations expect an extra padding. I would be curious to hear more
> > thoughts from others here.
> >
> > Saying that, calling a new routine something like add_shmem_align_size
> > makes it clear what's its purpose.
>
> My limited opinion on the matter after spending some time yesterday through
> the related code, is that with the current api it is easy to miss something
> and underestimate or just be sloppy. If only from the readability point of
> view, adding the proposed add_shmem_align_size will be beneficial.
>
> I hold no opinion on backpatching.

So here's a v2 patch which hopefully account for all unaccounted alignment
padding. There's also a significant change in the shared hash table size
estimation. AFAICT, allocation will be done this way:

- num_freelists allocs of init_size / num_freelists entries (on average) for
partitioned htab, num_freelist being 1 for non partitioned table,
NUM_FREELISTS (32) otherwise.

- then the rest of the entries, if any, are alloced in groups of
choose_nelem_alloc() entries

With this patch applied, I see an extra 16KB of shared memory being requested.