public inbox for [email protected]  

help / color / mirror / Atom feed

From: Reid Thompson <[email protected]>
To: John Morris <[email protected]>
To: vignesh C <[email protected]>
To: Arne Roland <[email protected]>
To: [email protected] <[email protected]>
To: Justin Pryzby <[email protected]>
To: Ibrar Ahmed <[email protected]>
To: stephen.frost <[email protected]>
Subject: Re: FW: Add the ability to limit the amount of memory that can be allocated to backends.
Date: Fri, 31 Mar 2023 09:39:00 -0400
Message-ID: <[email protected]> (raw)
In-Reply-To: <BYAPR13MB2677432D0680469210C3542DA08E9@BYAPR13MB2677.namprd13.prod.outlook.com>
References: <BYAPR13MB2677432D0680469210C3542DA08E9@BYAPR13MB2677.namprd13.prod.outlook.com>

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

view thread (2+ messages)

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: FW: Add the ability to limit the amount of memory that can be allocated to backends.
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox