On Thu, Sep 1, 2016 at 12:14 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I still think it'd be better to fix that as attached, because it
> represents a net reduction not net addition of code, and it provides
> a defense against future repetitions of the same omission. If only
> 4 out of 11 existing calls were properly checked --- some of them
> adjacent to calls with checks --- that should tell us that we *will*
> have more instances of the same bug if we don't fix it centrally.
>
> I also note that your patch missed checks for two ShmemAlloc calls in
> InitShmemAllocation and ShmemInitStruct. Admittedly, since those are
> the very first such calls, it's highly unlikely they'd fail; but I think
> this exercise is not about dismissing failures as improbable. Almost
> all of these failures are improbable, given that we precalculate the
> shmem space requirement.
OK, that looks fine to me after review.
Also, we could take one extra step forward then, and just introduce
ShmemAllocExtended that holds two flags as per the attached:
- SHMEM_ALLOC_ZERO that zeros all the fields
- SHMEM_ALLOC_NO_OOM that does not fail
Or we could just put a call to MemSet directly in ShmemAlloc(), but
I'd rather keep the base routines extensible.
What do you think about the attached? One other possibility would be
to take your patch, but use MemSet unconditionally on it as this
should not cause any overhead.
--
Michael