Re: Shared memory size computation oversight?

Hi,

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Saturday, February 27, 2021 9:08 AM, Julien Rouhaud <rjuju123(at)gmail(dot)com> wrote:

> Hi,
>
> While investigating on some strange "out of shared memory" error reported on
> the french BBS [1], I noticed that 09adc9a8c09 (adding Robert in Cc) changed
> ShmemAlloc alignment to CACHELINEALIGN but didn't update any related code that
> depended on it.

Nice catch.

>
> Most of the core code isn't impacted as it doesn't have to reserve any shared
> memory, but AFAICT pg_prewarm and pg_stat_statements can now slightly
> underestimate the amount of shared memory they'll use, and similarly for any
> third party extension that still rely on MAXALIGN alignment. As a consequence
> those extension can consume a few hundred bytes more than they reserved, which
> probably will be borrowed from the lock hashtables reserved size that isn't
> alloced immediately. This can later lead to ShmemAlloc failing when trying to
> acquire a lock while the hash table is almost full but should still have enough
> room to store it, which could explain the error reported.
>
> PFA a patch that fixes pg_prewarm and pg_stat_statements explicit alignment to
> CACHELINEALIGN, and also update the alignment in hash_estimate_size() to what I
> think ShmemInitHash will actually consume.

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.

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).

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.

Thank you for your patience,
//Georgios -- https://www.vmware.com

>
> [1] https://forums.postgresql.fr/viewtopic.php?pid=32138#p32138 sorry, it's all
> in French.