Re: Shared memory size computation oversight?

On Wed, Mar 03, 2021 at 03:37:20PM +0000, Georgios wrote:
>
> Please excuse me as I speak most from the side of ignorance. I am slightly curious
> to understand something in your patch, if you can be kind enough to explain it to
> me.
>
> The commit 09adc9a8c09 you pointed out to, as far as I can see, changed the total
> size of the shared memory, not individual bits. It did explain that the users of
> that space had properly padded their data, but even assuming that they did not do
> that as asked, the result would (or rather should) be cache misses, not running out
> of reserved memory.

It actually changed the allocation of individual bits inside the fixed size
shared memory, as underlying users ends up calling shmemalloc(),
but did not accurately changed the actual total size of shared memory as many
estimates are done either still using MAXALIGN or simply not accounting for the
padding.

> My limited understanding is also based in a comment in CreateSharedMemoryAndSemaphores()
>
> /*
> * Size of the Postgres shared-memory block is estimated via
> * moderately-accurate estimates for the big hogs, plus 100K for the
> * stuff that's too small to bother with estimating.
> *
> * We take some care during this phase to ensure that the total size
> * request doesn't overflow size_t. If this gets through, we don't
> * need to be so careful during the actual allocation phase.
> */
> size = 100000;
>
> Of course, my argument falls bare, if the size estimates of each of the elements is
> rather underestimated. Indeed this is the argument in the present patch expressed in
> code in hash_estimate_size most prominently, here:
>
> size = add_size(size,
> - mul_size(nElementAllocs,
> - mul_size(elementAllocCnt, elementSize)));
> + CACHELINEALIGN(Nmul_size(nElementAllocs,
> + mul_size(elementAllocCnt, elementSize))));
>
> (small note, Nmul_size is a typo of mul_size, but that is minor, by amending it the
> code compiles).

Oops, I'll fix that.

> To conclude, the running out of shared memory, seems to me to be fixed rather
> vaguely with this patch. I am not claiming that increasing the memory used by
> the elements is not needed, I am claiming that I can not clearly see how is that
> specific increase needed.

I'm also not entirely convinced that those fixes are enough to explain the "out
of shared memory" issue originally reported, but that's the only class of
problem that I could spot which could possibly explain it.

As Robert initially checked, it should be at worse a 6kB understimate with
default parameters (it may be slightly more now as we have more shared memory
users, but it should be the same order of magnitude), and I don't think that
there it will vary a lot with huge shared_buffers and/or max_connections.

Those 6kB should be compared to how much room is giving the initial 100k vs how
much the small stuff is actually computing.

Note that there are still some similar issue in the code. A quick look at
ProcGlobalShmemSize() vs InitProcGlobal() seems to indicate that it's missing 5
CACHELINEALIGN in the estimate. Unless someone object, I'll soon do a full
review of all the estimates in CreateSharedMemoryAndSemaphores() and fix all
additional occurences that I can find.