Andres Freund <andres@anarazel.de> writes:
> On 2020-04-05 15:04:30 -0400, Tom Lane wrote:
>> That would only reduce the chance of getting a stack overflow there,
>> and not by that much, especially not for a CLOBBER_CACHE_ALWAYS animal
>> which is going to be doing catalog accesses inside there too.
> It'd certainly not be bullet proof. But I don't think we ever were? If I
> understood you correctly we were just not noticing the stack overflow
> danger before? We did catalog accesses from within there before too,
> that's not changed by the addition of equal(), no?
Ah, you're right, the CCA aspect is not such a problem as long as there
are not check_stack_depth() calls inside the code that's run to load a
syscache or relcache entry. Which there probably aren't, at least not
for system catalogs. The reason we're seeing this on a CCA animal is
simply that a cache flush has occurred to force recomputeNamespacePath
to do some work. (In theory it could happen on a non-CCA animal, given
unlucky timing of an sinval overrun.)
My point here though is that it's basically been blind luck that we've
not seen this before. There's certainly no good reason to assume that
a check_stack_depth() call shouldn't happen while parsing a function
call, or within some other chunk of the parser that happens to set up
a transient error-position callback. And it's only going to get more
likely in future, seeing for example my ambitions to extend the
executor so that run-time expression failures can also report error
cursors. So I think that we should be looking for a permanent fix,
not a reduce-the-odds band-aid.
>> Seems like that adds a lot of potential for memory leakage?
> Depends on the case, I'd say. Certainly might be useful to add a helper
> for a corresponding conditional free.
> For parsing cases like this it could be better to bulk free at the
> end. Compared to the memory needed for all the transformed arguments etc
> it'd probably not matter in the short term (especially if only done for
> 4+ args).
What I wish we had was alloca(), so you don't need a FUNC_MAX_ARGS-sized
array to parse a two-argument function call. Too bad C99 didn't add
that. (But some sniffing around suggests that an awful lot of systems
have it anyway ... even MSVC. Hmmm.)
regards, tom lane