Re: FW: Add the ability to limit the amount of memory that can be allocated to backends. - Mailing list pgsql-hackers

 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