From a680e6b6885378beb0164e465b50afd81558ebc5 Mon Sep 17 00:00:00 2001 From: Tom Lane 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 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