Re: 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