Renaming MemoryContextResetAndDeleteChildren to MemoryContextReset - Mailing list pgsql-hackers

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);