Re: Shared memory size computation oversight?

On Fri, Aug 13, 2021 at 10:52:50AM +0200, Peter Eisentraut wrote:
> On 12.08.21 16:18, Julien Rouhaud wrote:
> >
> > But changing RequestAddinShmemSpace() to apply CACHELINEALIGN() would
> > only really work for that specific usage only? If an extension does
> > multiple allocations you can't rely on correct results.
>
> I think you can do different things here to create inconsistent results, but
> I think there should be one common, standard, normal, straightforward way to
> get a correct result.

Unless I'm missing something, the standard and straightforward way to get a
correct result is to account for padding bytes in the C code, as it's currently
done in all contrib modules. The issue is that those modules aren't using the
correct alignment anymore.
>
> > > Btw., I think your patch was wrong to apply CACHELINEALIGN() to
> > > intermediate results rather than at the end.
> >
> > I'm not sure why you think it's wrong. It's ShmemInitStruct() that
> > will apply the padding, so if the extension calls it multiple times
> > (whether directly or indirectly), then the padding will be added
> > multiple times. Which means that in theory the extension should
> > account for it multiple time in the amount of memory it's requesting.
>
> Yeah, in that case it's probably rather the case that there is one
> CACHELINEALIGN() too few, since pg_stat_statements does two separate shmem
> allocations.

I still don't get it. Aligning the total shmem size is totally different from
properly padding all single allocation sizes, and is almost never the right
answer.

Using a naive example, if your extension needs to ShmemInitStruct() twice 64B,
postgres will consume 2*128B. But if you only rely on RequestAddinShmemSpace()
to add a CACHELINEALIGN(), then no padding at all will be added, and you'll
then be not one but two CACHELINEALIGN() too few.

But again, the real issue is not the CACHELINEALIGN() roundings, as those have
a more or less stable size and are already accounted for in the extra 100kB,
but the dynahash size estimation which seems to be increasingly off as the
number of entries grows.