Re: Expand palloc/pg_malloc API

Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com> writes:
> On 11.10.22 18:04, Tom Lane wrote:
>> Hmm ... the individual allocators have that info, but mcxt.c doesn't
>> have access to it. I guess we could invent an additional "method"
>> to return the requested size of a chunk, which is only available in
>> MEMORY_CONTEXT_CHECKING builds, or maybe in !MEMORY_CONTEXT_CHECKING
>> it returns the allocated size instead.

> I'm not sure whether that amount of additional work would be useful
> relative to the size of this patch. Is the patch as it stands now
> making the code less robust than what the code is doing now?

No. It's slightly annoying that the call sites still have to track
the old size of the allocation, but I guess that they have to have
that information in order to know that they need to repalloc in the
first place. I agree that this patch does make things easier to
read and a bit less error-prone.

Also, I realized that what I proposed above doesn't really work
anyway for this purpose. Consider

ptr = palloc(size);
... fill all "size" bytes ...
ptr = repalloc0(ptr, size, newsize);

where the initial size request isn't a power of 2. If production builds
rely on the initial allocated size not requested size to decide where to
start zeroing, this would work (no uninitialized holes) in a debug build,
but leave some uninitialized bytes in a production build, which is
absolutely horrible. So I guess we have to rely on the callers to
track their requests.

> In the meantime, here is an updated patch with the argument order
> swapped and an additional assertion, as previously discussed.

I think it'd be worth expending an actual runtime test in repalloc0,
that is

if (unlikely(oldsize > size))
elog(ERROR, "invalid repalloc0 call: oldsize %zu, new size %zu",
oldsize, size);

This seems cheap compared to the cost of the repalloc+memset, and the
consequences of not detecting the error seem pretty catastrophic ---
the memset would try to zero almost your whole address space.

No objections beyond that. I've marked this RFC.

regards, tom lane