Re: Expand palloc/pg_malloc API

Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> On 17.05.22 20:43, Tom Lane wrote:
>> So I think Peter's got a good idea here (I might quibble with the details
>> of some of these macros).  But it's not really going to move the
>> safety goalposts very far unless we make a concerted effort to make
>> these be the style everywhere.  Are we willing to do that?  What
>> will it do to back-patching difficulty?  Dare I suggest back-patching
>> these changes?

> I think it could go like the castNode() introduction: first we adopt it 
> sporadically for new code, then we change over some larger pieces of 
> code, then we backpatch the API, then someone sends in a big patch to 
> change the rest.

OK, that seems like a reasonable plan.

I've now studied this a little more closely, and I think the
functionality is fine, but I have naming quibbles.

1. Do we really want distinct names for the frontend and backend
versions of the macros?  Particularly since you're layering the
frontend ones over pg_malloc, which has exit-on-OOM behavior?
I think we've found that notational discrepancies between frontend
and backend code are generally more a hindrance than a help,
so I'm inclined to drop the pg_malloc_xxx macros and just use
"palloc"-based names across the board.

2. I don't like the "palloc_ptrtype" name at all.  I see that you
borrowed that name from talloc, but I doubt that's a precedent that
very many people are familiar with.  To me it sounds like it might
allocate something that's the size of a pointer, not the size of the
pointed-to object.  I have to confess though that I don't have an
obviously better name to suggest.  "palloc_pointed_to" would be
clear perhaps, but it's kind of long.

3. Likewise, "palloc_obj" is perhaps less clear than it could be.
I find palloc_array just fine though.  Maybe palloc_object or
palloc_struct?  (If "_obj" can be traced to talloc, I'm not
seeing where.)


One thought that comes to mind is that palloc_ptrtype is almost
surely going to be used in the style

    myvariable = palloc_ptrtype(myvariable);

and if it's not that it's very likely wrong.  So maybe we should cut
out the middleman and write something like

#define palloc_instantiate(ptr) ((ptr) = (typeof(ptr)) palloc(sizeof(*(ptr))))
...
    palloc_instantiate(myvariable);

I'm not wedded to "instantiate" here, there's probably better names.

            regards, tom lane