Re: Rethinking MemoryContext creation

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Mon, Dec 11, 2017 at 12:36 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> [ thinks... ] If we wanted to go that way, one thing we could do to
>> help extension authors (and ourselves) is to define the proposed
>> AllocSetContextCreate macro to include
>>
>> StaticAssertExpr(__builtin_constant_p(name))
>>
>> on compilers that have __builtin_constant_p. Now, that only helps
>> people using gcc and gcc-alikes, but that's a large fraction of
>> developers I should think. (I tested this and it does seem to
>> correctly recognize string literals as constants.)

> I like that idea. I think that would provide good protection not only
> for third-party developers but for core developers.

It turns out this is slightly more painful than I'd anticipated.
I tried to #define AllocSetContextCreate with five parameters,
but all of the call sites that use the size abstraction macros
(ALLOCSET_DEFAULT_SIZES and friends) blew up, because as far as
they were concerned there were only three parameters, since the
abstraction macros hadn't gotten expanded yet.

We can make it work by #defining AllocSetContextCreate with three
parameters

#define AllocSetContextCreate(parent, name, allocparams) ...

This approach means that you *must* use an abstraction macro when going
through AllocSetContextCreate; if you want to write out the parameters
longhand, you have to call AllocSetContextCreateExtended. I do not
feel that this is a big loss, but there were half a dozen sites in our
code that were doing it the old way. More significantly, since we
only introduced those macros in 9.6, I suspect that most extensions
are still doing it the old way and will get broken by this change.
It's not hard to fix, but the annoyance factor will probably be real.
I see no good way around it though: we can't use a static inline
function instead, because that will almost certainly break the
__builtin_constant_p test.

I did not bother with compatibility macros for SlabContext or
GenerationContext --- I really doubt any extension code is using
the former yet, and they couldn't be using the latter since it's new.

I've not done any benchmarking on this yet, just confirmed that it
compiles and passes check-world.

regards, tom lane