FW: Add the ability to limit the amount of memory that can be allocated to backends.

On Thu, 2023-03-30 at 16:11 +0000, John Morris wrote:

 Hi Reid!
Some thoughts
I was looking at lmgr/proc.c, and I see a potential integer overflow
- bothmax_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 whenmax_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
 
 
 
 

John,
Thanks for looking this over and catching this. I appreciate the catch
and the guidance. 

Thanks,
Reid