Re: [HACKERS] Renaming MemoryContextResetAndDeleteChildren to MemoryContextReset
Andrew Gierth writes: > "Tom" == Tom Lane 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Renaming MemoryContextResetAndDeleteChildren to MemoryContextReset
> "Tom" == Tom Lane 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.ca...@dell.linuxdev.us.dell.com Tom> It's come up again every time we found another leak of dead child Tom> contexts, which happened twice last year for example. And that Tom> patch I'm pushing for "expanded" out-of-line objects really needs Tom> this to become the default behavior anywhere that we can detoast Tom> 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) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Renaming MemoryContextResetAndDeleteChildren to MemoryContextReset
On 2015-02-26 18:05:46 -0500, Tom Lane wrote: > Andres Freund 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Renaming MemoryContextResetAndDeleteChildren to MemoryContextReset
Andres Freund 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Renaming MemoryContextResetAndDeleteChildren to MemoryContextReset
Andres Freund 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Renaming MemoryContextResetAndDeleteChildren to MemoryContextReset
Andres Freund 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Renaming MemoryContextResetAndDeleteChildren to MemoryContextReset
On 2015-02-26 17:45:26 -0500, Tom Lane wrote: > Andres Freund 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Renaming MemoryContextResetAndDeleteChildren to MemoryContextReset
Andres Freund 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.ca...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Renaming MemoryContextResetAndDeleteChildren to MemoryContextReset
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Renaming MemoryContextResetAndDeleteChildren to MemoryContextReset
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.ca...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Renaming MemoryContextResetAndDeleteChildren to MemoryContextReset
Alvaro Herrera 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Renaming MemoryContextResetAndDeleteChildren to MemoryContextReset
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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] 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.ca...@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 wan