Re: Safe memory allocation functions

On 2015-01-16 23:06:12 +0900, Michael Paquier wrote:
>  /*
> + * Wrappers for allocation functions.
> + */
> +static void *set_alloc_internal(MemoryContext context,
> +                                Size size, bool noerror);
> +static void *set_realloc_internal(MemoryContext context, void *pointer,
> +                                  Size size, bool noerror);
> +
> +/*
>   * These functions implement the MemoryContext API for AllocSet contexts.
>   */
>  static void *AllocSetAlloc(MemoryContext context, Size size);
> +static void *AllocSetAllocNoError(MemoryContext context, Size size);
>  static void AllocSetFree(MemoryContext context, void *pointer);
>  static void *AllocSetRealloc(MemoryContext context, void *pointer, Size size);
> +static void *AllocSetReallocNoError(MemoryContext context,
> +                                 void *pointer, Size size);
>  static void AllocSetInit(MemoryContext context);
>  static void AllocSetReset(MemoryContext context);
>  static void AllocSetDelete(MemoryContext context);
> @@ -264,8 +275,10 @@ static void AllocSetCheck(MemoryContext context);
>   */
>  static MemoryContextMethods AllocSetMethods = {
>      AllocSetAlloc,
> +    AllocSetAllocNoError,
>      AllocSetFree,
>      AllocSetRealloc,
> +    AllocSetReallocNoError,
>      AllocSetInit,
>      AllocSetReset,
>      AllocSetDelete,
> @@ -517,140 +530,16 @@ AllocSetContextCreate(MemoryContext parent,
>  }

Wouldn't it make more sense to change the MemoryContext API to return
NULLs in case of allocation failure and do the error checking in the
mcxt.c callers?
> +/* wrapper routines for allocation */
> +static void* palloc_internal(Size size, bool noerror);
> +static void* repalloc_internal(void *pointer, Size size, bool noerror);
> +
>  /*
>   * You should not do memory allocations within a critical section, because
>   * an out-of-memory error will be escalated to a PANIC. To enforce that
> @@ -684,8 +688,8 @@ MemoryContextAllocZeroAligned(MemoryContext context, Size size)
>      return ret;
>  }
>  
> -void *
> -palloc(Size size)
> +static void*
> +palloc_internal(Size size, bool noerror)
>  {
>      /* duplicates MemoryContextAlloc to avoid increased overhead */
>      void       *ret;
> @@ -698,31 +702,85 @@ palloc(Size size)
>  
>      CurrentMemoryContext->isReset = false;
>  
> -    ret = (*CurrentMemoryContext->methods->alloc) (CurrentMemoryContext, size);
> +    if (noerror)
> +        ret = (*CurrentMemoryContext->methods->alloc_noerror)
> +            (CurrentMemoryContext, size);
> +    else
> +        ret = (*CurrentMemoryContext->methods->alloc)
> +            (CurrentMemoryContext, size);
>      VALGRIND_MEMPOOL_ALLOC(CurrentMemoryContext, ret, size);
>  
>      return ret;
>  }

I'd be rather surprised if these branches won't show up in
profiles. This is really rather hot code. At the very least this helper
function should be inlined. Also, calling the valgrind function on an
allocation failure surely isn't correct.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services