Re: Renaming MemoryContextResetAndDeleteChildren to MemoryContextReset

On 2015-02-26 17:45:26 -0500, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> Or are you arguing for an alternative proposal in which we remove
> MemoryContextReset (or at least rename it to something new) and thereby
> intentionally break all code that uses MemoryContextReset?

Yes, that I am.

After a bit of thought I just sent the suggestion to add a parameter to
MemoryContextReset(). That way the compiler will warn.

> > ... Without a compiler erroring out people won't
> > notice that suddenly MemoryContextReset deletes much more; leading to
> > possibly hard to find errors. Context resets frequently are in error
> > paths, and those won't necessarily be hit when running with assertions
> > enabled.
>
> With all due respect, that's utterly wrong. I have looked at every single
> MemoryContextReset call in the codebase, and as far as I can see the
> *only* one that is in an error path is elog.c:336, which I'm quite certain
> is wrong anyway (the other reset of ErrorContext in that file uses
> MemoryContextResetAndDeleteChildren, this one should too).

Sure, in the pg codebase. But there definitely are extensions using
memory contexts. And there's code that has to work across several
branches. Backpatching alone imo presents a risk; there's nothing to
warn you three years down the road that the MemoryContextReset() you
just backpatched doesn't work the same in the backbranches.

If the changes breaks some code it's likely actually a good thing:
Because, as you say, using MemoryContextReset() will likely be the wrong
thing, and they'll want to fix it for the existing branches as well.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services