Thread: AllocSetContextCreate changes breake extensions
Hi, Christoph Berg, on IRC, raised the issue that at least one extension failed compiling in v11. The extension currently does: https://github.com/pgq/pgq/blob/master/triggers/common.c#L225 tbl_cache_ctx = AllocSetContextCreate(TopMemoryContext, "pgq_triggers table info", ALLOCSET_SMALL_MINSIZE, ALLOCSET_SMALL_INITSIZE, ALLOCSET_SMALL_MAXSIZE); which makes sense, because it has to support versions below 9.6, which introduced ALLOCSET_SMALL_SIZES etc. But 9fa6f00b1308dd10da4eca2f31ccbfc7b35bb461 / Rethink MemoryContext creation to improve performance causes this to fail to compile because since then AllocSetContextCreate is declared to have three parameters: #ifdef HAVE__BUILTIN_CONSTANT_P #define AllocSetContextCreate(parent, name, allocparams) \ (StaticAssertExpr(__builtin_constant_p(name), \ "memory context names must be constant strings"), \ AllocSetContextCreateExtended(parent, name, allocparams)) #else #define AllocSetContextCreate(parent, name, allocparams) \ AllocSetContextCreateExtended(parent, name, allocparams) #endif which means it only compiles if ALLOCSET_*_SIZES is passed, rather than individual parameters. I think we should fix that. If the goal was to only allow passing the *SIZES parameters, we should have called it out as that. Based on a quick look, ISTM the easiest fix is to have the AllocSetContextCreate accept five parameters, and move it below the ALLOCSET_*_SIZES macros. That way they should be expanded before AllocSetContextCreate(), and thus 5 params should be fine. Greetings, Andres Freund
Re: Andres Freund 2018-10-12 <20181012170355.bhxi273skjt6sag4@alap3.anarazel.de> > Hi, > > Christoph Berg, on IRC, raised the issue that at least one extension > failed compiling in v11. The extension currently does: > https://github.com/pgq/pgq/blob/master/triggers/common.c#L225 Others have already been fixed, e.g. hll: https://github.com/citusdata/postgresql-hll/pull/52/commits/e7bfbc80bbaca547167d645be11db24c8922385f Andres' idea would enable the old code to continue to work, but couldn't we additionally to backpatch the ALLOCSET_*_SIZES macros, so the new code works also on old versions that didn't get the new AllocSetContextCreate macro? Christoph
Andres Freund <andres@anarazel.de> writes: > Christoph Berg, on IRC, raised the issue that at least one extension > failed compiling in v11. The extension currently does: > https://github.com/pgq/pgq/blob/master/triggers/common.c#L225 > tbl_cache_ctx = AllocSetContextCreate(TopMemoryContext, > "pgq_triggers table info", > ALLOCSET_SMALL_MINSIZE, > ALLOCSET_SMALL_INITSIZE, > ALLOCSET_SMALL_MAXSIZE); > which makes sense, because it has to support versions below 9.6, which > introduced ALLOCSET_SMALL_SIZES etc. Yeah, we discussed that at the time and thought it was acceptable collateral damage. It's not like nobody ever breaks API in new major versions. > Based on a quick look, ISTM the easiest fix is to have the > AllocSetContextCreate accept five parameters, and move it below the > ALLOCSET_*_SIZES macros. That way they should be expanded before > AllocSetContextCreate(), and thus 5 params should be fine. Huh? The order in which you #define macros doesn't affect expansion. We could make this work more conveniently on compilers supporting __VA_ARGS__, though. That's certainly good enough in HEAD, and probably good enough for most people in 11. regards, tom lane
On 10/12/2018 01:10 PM, Christoph Berg wrote: > Re: Andres Freund 2018-10-12 > Andres' idea would enable the old code to continue to work, but > couldn't we additionally to backpatch the ALLOCSET_*_SIZES macros, so > the new code works also on old versions that didn't get the new > AllocSetContextCreate macro? That's effectively what PL/Java did, just for itself. Was pretty straightforward. https://github.com/tada/pljava/commit/3b67999 -Chap
On 2018-10-12 13:20:16 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > Christoph Berg, on IRC, raised the issue that at least one extension > > failed compiling in v11. The extension currently does: > > https://github.com/pgq/pgq/blob/master/triggers/common.c#L225 > > tbl_cache_ctx = AllocSetContextCreate(TopMemoryContext, > > "pgq_triggers table info", > > ALLOCSET_SMALL_MINSIZE, > > ALLOCSET_SMALL_INITSIZE, > > ALLOCSET_SMALL_MAXSIZE); > > > which makes sense, because it has to support versions below 9.6, which > > introduced ALLOCSET_SMALL_SIZES etc. > > Yeah, we discussed that at the time and thought it was acceptable > collateral damage. It's not like nobody ever breaks API in new major > versions. Sure, we do that all the time. It just seems quite unnecessarily painful here, especially because ALLOCSET_*_SIZES wasn't backpatched. > > Based on a quick look, ISTM the easiest fix is to have the > > AllocSetContextCreate accept five parameters, and move it below the > > ALLOCSET_*_SIZES macros. That way they should be expanded before > > AllocSetContextCreate(), and thus 5 params should be fine. > > Huh? The order in which you #define macros doesn't affect expansion. return -ENOCOFFEE; But can't we just do something like: #if defined(HAVE__BUILTIN_CONSTANT_P) && defined(HAVE__VA_ARGS) #define AllocSetContextCreate(parent, name, ...) \ (StaticAssertExpr(__builtin_constant_p(name), \ "memory context names must be constant strings"), \ AllocSetContextCreateExtended(parent, name, __VA_ARGS__)) #else #define AllocSetContextCreate \ AllocSetContextCreateExtended #endif The set of compilers that have __builtin_constant_p and not vararg macros got to be about empty. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> But can't we just do something like:
> #if defined(HAVE__BUILTIN_CONSTANT_P) && defined(HAVE__VA_ARGS)
> #define AllocSetContextCreate(parent, name, ...) \
> (StaticAssertExpr(__builtin_constant_p(name), \
> "memory context names must be constant strings"), \
> AllocSetContextCreateExtended(parent, name, __VA_ARGS__))
> #else
> #define AllocSetContextCreate \
> AllocSetContextCreateExtended
> #endif
> The set of compilers that have __builtin_constant_p and not vararg
> macros got to be about empty.
Yeah, fair point, and anyway we don't need the StaticAssert to exist
everywhere. I'll make it so.
Shall we also backpatch the ALLOCSET_*_SIZES macros as Christoph
suggested? I'm not convinced of the usefulness of that, since
extensions would still have to cope with them not being present
when building against existing minor releases.
regards, tom lane
On 2018-10-12 13:51:53 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > But can't we just do something like: > > > #if defined(HAVE__BUILTIN_CONSTANT_P) && defined(HAVE__VA_ARGS) > > #define AllocSetContextCreate(parent, name, ...) \ > > (StaticAssertExpr(__builtin_constant_p(name), \ > > "memory context names must be constant strings"), \ > > AllocSetContextCreateExtended(parent, name, __VA_ARGS__)) > > #else > > #define AllocSetContextCreate \ > > AllocSetContextCreateExtended > > #endif > > > The set of compilers that have __builtin_constant_p and not vararg > > macros got to be about empty. > > Yeah, fair point, and anyway we don't need the StaticAssert to exist > everywhere. I'll make it so. Cool. > Shall we also backpatch the ALLOCSET_*_SIZES macros as Christoph > suggested? I'm not convinced of the usefulness of that, since > extensions would still have to cope with them not being present > when building against existing minor releases. I'd do so. Many extensions are fine just building against a relatively new minor release. Won't help extension authors in the next few months, but after that... Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> On 2018-10-12 13:51:53 -0400, Tom Lane wrote:
>> Shall we also backpatch the ALLOCSET_*_SIZES macros as Christoph
>> suggested? I'm not convinced of the usefulness of that, since
>> extensions would still have to cope with them not being present
>> when building against existing minor releases.
> I'd do so. Many extensions are fine just building against a relatively
> new minor release. Won't help extension authors in the next few months,
> but after that...
I'm still not very convinced, but it's easy and harmless, so done.
regards, tom lane
>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
[snip]
The commit for this said:
With this change, there is no reason for anybody to call
AllocSetContextCreateExtended directly, so in HEAD I renamed it to
except there IS such a reason: if you need (as I do in pl/lua) to wrap
the call in a catch block, inside a function which takes the name and so
on as a parameter, then you have no option but to do so (since using the
macro errors out on the non-const parameter).
Right now I'm stuck with this:
PLLUA_TRY();
{
#if PG_VERSION_NUM >= 120000
mcxt = AllocSetContextCreateInternal(parent, name, minsz, initsz, maxsz);
#elif PG_VERSION_NUM >= 110000
mcxt = AllocSetContextCreateExtended(parent, name, minsz, initsz, maxsz);
#else
mcxt = AllocSetContextCreate(parent, name, minsz, initsz, maxsz);
#endif
*p = mcxt;
}
PLLUA_CATCH_RETHROW();
which kind of sucks. At least let's revert the pointless name change.
--
Andrew (irc:RhodiumToad)
Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
> With this change, there is no reason for anybody to call
> AllocSetContextCreateExtended directly, so in HEAD I renamed it to
> except there IS such a reason: if you need (as I do in pl/lua) to wrap
> the call in a catch block, inside a function which takes the name and so
> on as a parameter, then you have no option but to do so (since using the
> macro errors out on the non-const parameter).
I'm kind of unimpressed by your example, because you're deliberately
breaking the safety check the macro sets out to provide. With code
structure like this, it's impossible to be sure that what was passed to
the wrapper function is actually a constant string. You'd be better
off using the workaround the comment suggests, which is to just pass ""
to AllocSetContextCreate and then use MemoryContextSetIdentifier to
copy the passed string.
> At least let's revert the pointless name change.
I don't think it's entirely pointless: it's keeping the "Extended"
name available for possible future use. If I revert, what name
are we going to use when we really do need an API-incompatible
version of AllocSetContextCreate?
regards, tom lane
>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes: >> except there IS such a reason: if you need (as I do in pl/lua) to >> wrap the call in a catch block, inside a function which takes the >> name and so on as a parameter, then you have no option but to do so >> (since using the macro errors out on the non-const parameter). Tom> I'm kind of unimpressed by your example, because you're Tom> deliberately breaking the safety check the macro sets out to Tom> provide. Yes, because in this case the names really _are_ constant strings, but that fact can't be exposed to AllocSetContextCreate without duplicating code all over the place. Tom> With code structure like this, it's impossible to be sure that Tom> what was passed to the wrapper function is actually a constant Tom> string. It's impossible for AllocSetContextCreate to be sure of that, it's not impossible for _me_ to be sure of that. (I could add my own macro with a __builtin_constant_p check if I felt the need.) Tom> You'd be better off using the workaround the comment suggests, Tom> which is to just pass "" to AllocSetContextCreate and then use Tom> MemoryContextSetIdentifier to copy the passed string. Copying the string would be overkill since it really is a constant. (And I can't copy the string into the context it identifies, because that would block use of MemoryContextReset. I'd have to copy it somewhere else - and then freeing it becomes much more tricky.) -- Andrew (irc:RhodiumToad)