Hi Reid!
Some thoughts
I was looking at lmgr/proc.c, and I see a potential integer overflow - both
max_total_bkend_mem and result are declared as “int”, so the expression
“max_total_bkend_mem * 1024 * 1024 - result * 1024 * 1024” could have a problem
when max_total_bkend_mem is set to 2G or more.
/*
* Account for shared memory
size and initialize
* max_total_bkend_mem_bytes.
*/
pg_atomic_init_u64(&ProcGlobal->max_total_bkend_mem_bytes,
max_total_bkend_mem * 1024 * 1024 - result *
1024 * 1024);
As more of a style thing (and definitely not an error), the calling code might
look smoother if the memory check and error handling were moved into a helper
function, say “backend_malloc”. For example, the following calling code
/* Do not exceed maximum allowed memory allocation */
if
(exceeds_max_total_bkend_mem(Slab_CONTEXT_HDRSZ(chunksPerBlock)))
{
MemoryContextStats(TopMemoryContext);
ereport(ERROR,
(errcode(ERRCODE_OUT_OF_MEMORY),
errmsg("out of
memory - exceeds max_total_backend_memory"),
errdetail("Failed while creating memory context \"%s\".",
name)));
}
slab = (SlabContext *)
malloc(Slab_CONTEXT_HDRSZ(chunksPerBlock));
if (slab == NULL)
….
Could become a single line:
Slab = (SlabContext *) backend_malloc(Slab_CONTEXT_HDRSZ(chunksPerBlock);
Note this is a change in error handling as backend_malloc() throws memory
errors. I think this change is already happening, as the error throws you’ve
already added are potentially visible to callers (to be verified). It also
could result in less informative error messages without the specifics of “while
creating memory context”. Still, it pulls repeated code into a single wrapper
and might be worth considering.
I do appreciate the changes in updating the global memory counter. My
recollection is the original version updated stats with every allocation, and
there was something that looked like a spinlock around the update. (My memory
may be wrong …). The new approach eliminates contention, both by having fewer
updates and by using atomic ops. Excellent.
I also have some thoughts about simplifying the atomic update logic you are
currently using. I need to think about it a bit more and will get back to you
later on that.
* John Morris
