v2-0010-Partially-fix-some-extremely-broken-code-from-52.patch

From a680e6b6885378beb0164e465b50afd81558ebc5 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Mon, 19 May 2025 00:02:20 -0400
Subject: [PATCH v2 10/20] Partially fix some extremely broken code from
 525392d57.

Avoid leaking memory in the stmt_context during BuildCachedPlan.
Sadly, this code has problems a lot worse than that (per the
documentation I added), so I suspect 525392d57 will get reverted
and we won't need this patch.

Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/285483.1746756246@sss.pgh.pa.us
---
 src/backend/utils/cache/plancache.c | 37 ++++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 8 deletions(-)

diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index 9bcbc4c3e97..40ba3e9df7c 100644
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -1109,22 +1109,32 @@ BuildCachedPlan(CachedPlanSource *plansource, List *qlist,
 	 */
 	if (!plansource->is_oneshot)
 	{
+		List	   *stmt_plist;
+
 		plan_context = AllocSetContextCreate(CurrentMemoryContext,
 											 "CachedPlan",
 											 ALLOCSET_START_SMALL_SIZES);
 		MemoryContextCopyAndSetIdentifier(plan_context, plansource->query_string);
 
-		stmt_context = AllocSetContextCreate(CurrentMemoryContext,
+		stmt_context = AllocSetContextCreate(plan_context,
 											 "CachedPlan PlannedStmts",
 											 ALLOCSET_START_SMALL_SIZES);
 		MemoryContextCopyAndSetIdentifier(stmt_context, plansource->query_string);
-		MemoryContextSetParent(stmt_context, plan_context);
 
+		/*
+		 * Copy plans into the stmt_context.
+		 */
 		MemoryContextSwitchTo(stmt_context);
-		plist = copyObject(plist);
+		stmt_plist = copyObject(plist);
 
+		/*
+		 * We actually need the top-level List object to be in the long-lived
+		 * plan_context, in case UpdateCachedPlan wants to update it; see
+		 * comments therein.  Do a shallow copy to make that happen.
+		 */
 		MemoryContextSwitchTo(plan_context);
-		plist = list_copy(plist);
+		plist = list_copy(stmt_plist);
+		list_free(stmt_plist);	/* be tidy */
 	}
 	else
 		plan_context = CurrentMemoryContext;
@@ -1251,12 +1261,22 @@ UpdateCachedPlan(CachedPlanSource *plansource, int query_index,
 
 	/*
 	 * Planning work is done in the caller's memory context.  The resulting
-	 * PlannedStmt is then copied into plan->stmt_context after throwing away
-	 * the old ones.
+	 * PlannedStmt(s) are then copied into plan->stmt_context after throwing
+	 * away the old ones.  But note that we re-use the long-lived
+	 * plan->stmt_list list to hold the pointers to the PlannedStmts.  This
+	 * kluge avoids breaking code that is iterating over that list, so long as
+	 * it's between statements and not currently using one of the contained
+	 * PlannedStmts.
+	 *
+	 * XXX this is, if not actively broken, at least unbelievably fragile.
+	 * Aside from the likelihood that the just-stated assumption doesn't hold
+	 * universally, there is not a good reason to believe that the length of
+	 * the plan list is constant.
 	 */
 	plan_list = pg_plan_queries(query_list, plansource->query_string,
 								plansource->cursor_options, NULL);
-	Assert(list_length(plan_list) == list_length(plan->stmt_list));
+	if (list_length(plan_list) != list_length(plan->stmt_list))
+		elog(ERROR, "UpdateCachedPlan(): plan list length changed");
 
 	MemoryContextReset(plan->stmt_context);
 	oldcxt = MemoryContextSwitchTo(plan->stmt_context);
@@ -1276,7 +1296,8 @@ UpdateCachedPlan(CachedPlanSource *plansource, int query_index,
 
 	/*
 	 * We've updated all the plans that might have been invalidated, so mark
-	 * the CachedPlan as valid.
+	 * the CachedPlan as valid.  XXX wrong: we could already have hit a new
+	 * invalidation event.
 	 */
 	plan->is_valid = true;
 
-- 
2.43.5