Thread: Renaming MemoryContextResetAndDeleteChildren to MemoryContextReset
We've discussed doing $SUBJECT off and on for nearly ten years, the oldest thread I could find about it being here: http://www.postgresql.org/message-id/flat/1186435268.16321.37.camel@dell.linuxdev.us.dell.com It's come up again every time we found another leak of dead child contexts, which happened twice last year for example. And that patch I'm pushing for "expanded" out-of-line objects really needs this to become the default behavior anywhere that we can detoast objects. So attached is a patch to do it. I've verified that this passes "make check-world", not that that proves all that much :-( I did not make an attempt to s/MemoryContextResetAndDeleteChildren/MemoryContextReset/ globally, as it's certainly unnecessary to do that for testing purposes. I'm not sure whether we should do so, or skip the code churn (there's about 30 occurrences in HEAD). We'd want to keep the macro in any case to avoid unnecessary breakage of 3rd-party code. Thoughts? Any objections to pushing this? regards, tom lane diff --git a/src/backend/utils/mmgr/README b/src/backend/utils/mmgr/README index 45e610d..e01bcd4 100644 *** a/src/backend/utils/mmgr/README --- b/src/backend/utils/mmgr/README *************** lifetimes that only partially overlap ca *** 125,133 **** from different trees of the context forest (there are some examples in the next section). ! For convenience we will also want operations like "reset/delete all ! children of a given context, but don't reset or delete that context ! itself". Globally Known Contexts --- 125,137 ---- from different trees of the context forest (there are some examples in the next section). ! Actually, it turns out that resetting a given context should almost ! always imply deleting (not just resetting) any child contexts it has. ! So MemoryContextReset() means that, and if you really do want a forest of ! empty contexts you need to call MemoryContextResetButPreserveChildren(). ! ! For convenience we also provide operations like "reset/delete all children ! of a given context, but don't reset or delete that context itself". Globally Known Contexts diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c index 202bc78..f40c33e 100644 *** a/src/backend/utils/mmgr/mcxt.c --- b/src/backend/utils/mmgr/mcxt.c *************** MemoryContextInit(void) *** 132,145 **** /* * MemoryContextReset * Release all space allocated within a context and its descendants, * but don't delete the contexts themselves. * * The type-specific reset routine handles the context itself, but we * have to do the recursion for the children. */ void ! MemoryContextReset(MemoryContext context) { AssertArg(MemoryContextIsValid(context)); --- 132,176 ---- /* * MemoryContextReset + * Release all space allocated within a context and delete all its + * descendant contexts (but not the named context itself). + * + * The type-specific reset routine handles the context itself, but we + * have to do the recursion for the children. + */ + void + MemoryContextReset(MemoryContext context) + { + AssertArg(MemoryContextIsValid(context)); + + /* save a function call in common case where there are no children */ + if (context->firstchild != NULL) + MemoryContextDeleteChildren(context); + + /* Nothing to do if no pallocs since startup or last reset */ + if (!context->isReset) + { + (*context->methods->reset) (context); + context->isReset = true; + VALGRIND_DESTROY_MEMPOOL(context); + VALGRIND_CREATE_MEMPOOL(context, 0, false); + } + } + + /* + * MemoryContextResetButPreserveChildren * Release all space allocated within a context and its descendants, * but don't delete the contexts themselves. * + * Note: this was formerly the behavior of plain MemoryContextReset(), but + * we found out that you almost always want to delete children not keep them. + * This API may indeed have no use at all, but we keep it for the moment. + * * The type-specific reset routine handles the context itself, but we * have to do the recursion for the children. */ void ! MemoryContextResetButPreserveChildren(MemoryContext context) { AssertArg(MemoryContextIsValid(context)); *************** MemoryContextResetChildren(MemoryContext *** 171,177 **** AssertArg(MemoryContextIsValid(context)); for (child = context->firstchild; child != NULL; child = child->nextchild) ! MemoryContextReset(child); } /* --- 202,208 ---- AssertArg(MemoryContextIsValid(context)); for (child = context->firstchild; child != NULL; child = child->nextchild) ! MemoryContextResetButPreserveChildren(child); } /* *************** MemoryContextDeleteChildren(MemoryContex *** 226,248 **** } /* - * MemoryContextResetAndDeleteChildren - * Release all space allocated within a context and delete all - * its descendants. - * - * This is a common combination case where we want to preserve the - * specific context but get rid of absolutely everything under it. - */ - void - MemoryContextResetAndDeleteChildren(MemoryContext context) - { - AssertArg(MemoryContextIsValid(context)); - - MemoryContextDeleteChildren(context); - MemoryContextReset(context); - } - - /* * MemoryContextSetParent * Change a context to belong to a new parent (or no parent). * --- 257,262 ---- diff --git a/src/include/utils/memutils.h b/src/include/utils/memutils.h index 85aba7a..65e291e 100644 *** a/src/include/utils/memutils.h --- b/src/include/utils/memutils.h *************** extern PGDLLIMPORT MemoryContext CurTran *** 84,89 **** --- 84,92 ---- /* This is a transient link to the active portal's memory context: */ extern PGDLLIMPORT MemoryContext PortalContext; + /* Backwards compatibility macro */ + #define MemoryContextResetAndDeleteChildren(ctx) MemoryContextReset(ctx) + /* * Memory-context-type-independent functions in mcxt.c *************** extern PGDLLIMPORT MemoryContext PortalC *** 91,99 **** extern void MemoryContextInit(void); extern void MemoryContextReset(MemoryContext context); extern void MemoryContextDelete(MemoryContext context); extern void MemoryContextResetChildren(MemoryContext context); extern void MemoryContextDeleteChildren(MemoryContext context); - extern void MemoryContextResetAndDeleteChildren(MemoryContext context); extern void MemoryContextSetParent(MemoryContext context, MemoryContext new_parent); extern Size GetMemoryChunkSpace(void *pointer); --- 94,102 ---- extern void MemoryContextInit(void); extern void MemoryContextReset(MemoryContext context); extern void MemoryContextDelete(MemoryContext context); + extern void MemoryContextResetButPreserveChildren(MemoryContext context); extern void MemoryContextResetChildren(MemoryContext context); extern void MemoryContextDeleteChildren(MemoryContext context); extern void MemoryContextSetParent(MemoryContext context, MemoryContext new_parent); extern Size GetMemoryChunkSpace(void *pointer);
Tom Lane wrote: > Thoughts? Any objections to pushing this? Is there any reason at all to keep MemoryContextResetButPreserveChildren()? Since your patch doesn't add any callers, it seems pretty likely that there's none anywhere. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Tom Lane wrote:
>> Thoughts? Any objections to pushing this?
> Is there any reason at all to keep
> MemoryContextResetButPreserveChildren()? Since your patch doesn't add
> any callers, it seems pretty likely that there's none anywhere.
The only reason to keep it is to have an "out" if it turns out that some
third-party code actually needs that behavior.
On reflection, maybe a better API to offer for that eventuality is a
function named something like MemoryContextResetOnly(), which would
leave child contexts completely alone. Then, if you want the old
functionality, you could get it with MemoryContextResetOnly plus
MemoryContextResetChildren.
BTW, the original thread discussed the idea of moving context bookkeeping
blocks into the immediate parent context, but the usefulness of
MemoryContextSetParent() negates the thought that that would be a good
plan. So there's no real issue here other than potential backwards
compatibility for external code.
regards, tom lane
Hi, On 2015-02-26 17:01:53 -0500, Tom Lane wrote: > We've discussed doing $SUBJECT off and on for nearly ten years, > the oldest thread I could find about it being here: > http://www.postgresql.org/message-id/flat/1186435268.16321.37.camel@dell.linuxdev.us.dell.com > It's come up again every time we found another leak of dead child > contexts, which happened twice last year for example. And that patch > I'm pushing for "expanded" out-of-line objects really needs this to > become the default behavior anywhere that we can detoast objects. I don't object to the behavioural change per se, rather the contrary, but I find it a pretty bad idea to change the meaning of an existing functioname this way. 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. Greetings, Andres Freund
On 2015-02-26 23:31:16 +0100, Andres Freund wrote: > 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. I'd really not even be surprised if a committer backpatches a MemoryContextReset() addition, not realizing it behaves differently in the back branches. How about MemoryContextReset(bool reset_children)? Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Andres Freund <andres@2ndquadrant.com> writes: > On 2015-02-26 17:01:53 -0500, Tom Lane wrote: >> We've discussed doing $SUBJECT off and on for nearly ten years, >> the oldest thread I could find about it being here: >> http://www.postgresql.org/message-id/flat/1186435268.16321.37.camel@dell.linuxdev.us.dell.com >> It's come up again every time we found another leak of dead child >> contexts, which happened twice last year for example. And that patch >> I'm pushing for "expanded" out-of-line objects really needs this to >> become the default behavior anywhere that we can detoast objects. > I don't object to the behavioural change per se, rather the contrary, > but I find it a pretty bad idea to change the meaning of an existing > functioname this way. That's pretty much the whole point I think. 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? I can't say that I find that an improvement. > ... 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). I see no reason whatever to believe that this change is likely to do anything except fix heretofore-unnoticed memory leaks. regards, tom lane
On 2015-02-26 17:45:26 -0500, Tom Lane wrote: > Andres Freund <andres@2ndquadrant.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
Andres Freund <andres@2ndquadrant.com> writes:
> ... Without a compiler erroring out people won't
> notice that suddenly MemoryContextReset deletes much more; leading to
> possibly hard to find errors.
BTW, so far as *data* is concerned, the existing call deletes all data in
the child contexts already. The only not-already-buggy operation you could
perform before that would no longer work is to allocate fresh data in one
of those child contexts, assuming you still had a pointer to such a
context. I've not seen any coding pattern in which that's likely. The
problem is exactly that whoever's resetting the parent context isn't aware
of child contexts having been attached to it.
regards, tom lane
Andres Freund <andres@2ndquadrant.com> writes:
> I'd really not even be surprised if a committer backpatches a
> MemoryContextReset() addition, not realizing it behaves differently in
> the back branches.
As far as that goes, the only consequence would be a possible memory leak
in the back branches; it would not be a really fatal problem. I'd rather
live with that risk than with the sort of intentional API break (and
ensuing back-patch pain) that you're proposing.
regards, tom lane
Andres Freund <andres@2ndquadrant.com> writes:
> On 2015-02-26 17:45:26 -0500, Tom Lane wrote:
>> 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.
Is that likely to happen? I doubt it. They'll just mutter under their
breath about useless API breakage and move on.
Basically, this is a judgment call, and I disagree with your judgment.
I think changing the behavior of MemoryContextReset is exactly what we
want to have happen. (And I've got to say that I'm losing patience with
backwards-compatibility arguments taken to this degree. We might as
well stop development entirely.)
regards, tom lane
On 2015-02-26 18:05:46 -0500, Tom Lane wrote: > Andres Freund <andres@2ndquadrant.com> writes: > > On 2015-02-26 17:45:26 -0500, Tom Lane wrote: > > 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. > > Is that likely to happen? I doubt it. They'll just mutter under their > breath about useless API breakage and move on. Maybe. > Basically, this is a judgment call, and I disagree with your judgment. Right. That's ok, happens all the time. > And I've got to say that I'm losing patience with > backwards-compatibility arguments taken to this degree. We might as > well stop development entirely. Meh, normally you're on the side I'm on right now... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes: Tom> We've discussed doing $SUBJECT off and on for nearly ten years,Tom> the oldest thread I could find about it being here:Tom>http://www.postgresql.org/message-id/flat/1186435268.16321.37.camel@dell.linuxdev.us.dell.comTom> It's come up againevery time we found another leak of dead childTom> contexts, which happened twice last year for example. And thatTom>patch I'm pushing for "expanded" out-of-line objects really needsTom> this to become the default behavior anywherethat we can detoastTom> objects. So, this is also changing (indirectly) the effect of ReScanExprContext so that deletes child contexts too. In the grouping sets work I noticed I had to explicitly add some MemoryContextDeleteChildren after ReScanExprContext in order to clean up properly; this obviously makes that redundant. (that looks like another data point in favour of this change) I guess the only question is whether anything currently relies on ReScanExprContext's current behavior. -- Andrew (irc:RhodiumToad)
Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
> Tom> We've discussed doing $SUBJECT off and on for nearly ten years,
> So, this is also changing (indirectly) the effect of ReScanExprContext
> so that deletes child contexts too.
Right, this is actually the main point so far as I'm concerned. My
"expanded arrays" patch currently has #define ResetExprContext(econtext) \
- MemoryContextReset((econtext)->ecxt_per_tuple_memory)
+ MemoryContextResetAndDeleteChildren((econtext)->ecxt_per_tuple_memory)
but this is a more general fix.
> I guess the only question is whether anything currently relies on
> ReScanExprContext's current behavior.
I've not seen any regression test failures either with the "expanded
arrays" patch or this one. It's conceivable that something would create a
context under a short-lived expression context and expect it to still be
there (though empty) after a context reset; that idea was the reason I
designed MemoryContextReset this way in the first place. But fifteen
years later, it's quite clear that that was a mistake and we don't
actually use contexts that way.
(Worth noting is that the memory context reset callback mechanism
I propose nearby is sort of a second pass at expression shutdown
callbacks, as well.)
regards, tom lane