From 603c2e3d20982bb0655c421d6ebe5f227af576cf Mon Sep 17 00:00:00 2001 From: Robert Haas Date: Tue, 7 Apr 2026 13:46:04 -0400 Subject: [PATCH v26 2/5] test_plan_advice: Guard against advice instability by replanning. It turns out that our main regression test suite queries tables upon which concurrent DDL is occurring, which can, rarely, cause test_plan_advice failures. For example, if test_plan_advice plans the query and generates INDEX_SCAN(a b) advice, and then the index is dropped before the query is replanned with that as the supplied advice, the advice will not apply cleanly and the tests will fail. Such failures are apparently quite rare, but they do occur. In an attempt to reduce the failure rate to something negligible, this commit makes test_plan_advice drive the main planner in a loop. We plan once, generating advice; then again, applying that advice. If the advice doesn't apply cleanly, we do the whole thing over again from the top. If it fails in the same way twice (same advice, same feedback) or if we run out of retries, we emit a warning and stop. Problems that change or vanish on retry are assumed to be ephemeral. Reported-by: Tom Lane --- .../test_plan_advice/t/001_replan_regress.pl | 2 +- .../test_plan_advice/test_plan_advice.c | 233 +++++++++++++++--- 2 files changed, 204 insertions(+), 31 deletions(-) diff --git a/src/test/modules/test_plan_advice/t/001_replan_regress.pl b/src/test/modules/test_plan_advice/t/001_replan_regress.pl index 38ffa4d11ae..e43b80bc85e 100644 --- a/src/test/modules/test_plan_advice/t/001_replan_regress.pl +++ b/src/test/modules/test_plan_advice/t/001_replan_regress.pl @@ -19,7 +19,7 @@ $node->init(); $node->append_conf('postgresql.conf', <start; diff --git a/src/test/modules/test_plan_advice/test_plan_advice.c b/src/test/modules/test_plan_advice/test_plan_advice.c index cff5039b5c8..c17115707b1 100644 --- a/src/test/modules/test_plan_advice/test_plan_advice.c +++ b/src/test/modules/test_plan_advice/test_plan_advice.c @@ -19,20 +19,38 @@ #include "postgres.h" #include "access/xact.h" +#include "commands/defrem.h" #include "fmgr.h" #include "optimizer/optimizer.h" +#include "optimizer/planner.h" #include "pg_plan_advice.h" #include "utils/guc.h" PG_MODULE_MAGIC; +static int test_plan_advice_max_attempts = 1; static bool in_recursion = false; +static void (*feedback_warning_fn) (List *feedback); +static planner_hook_type prev_planner_hook = NULL; +static PlannedStmt *test_plan_advice_planner(Query *parse, + const char *query_string, + int cursorOptions, + ParamListInfo boundParams, + ExplainState *es); static char *test_plan_advice_advisor(PlannerGlobal *glob, Query *parse, const char *query_string, int cursorOptions, ExplainState *es); +static PlannedStmt *copy_and_plan_query(Query *parse, + const char *query_string, + int cursorOptions, + ParamListInfo boundParams, + ExplainState *es, + bool suppress_messages); +static List *extract_feedback(PlannedStmt *pstmt); +static bool all_feedback_is_ok(List *feedback); static DefElem *find_defelem_by_defname(List *deflist, char *defname); /* @@ -43,6 +61,21 @@ _PG_init(void) { void (*add_advisor_fn) (pg_plan_advice_advisor_hook hook); + DefineCustomIntVariable("test_plan_advice.max_attempts", + "Maximum number of planning attempts before " + "reporting feedback warnings.", + NULL, + &test_plan_advice_max_attempts, + 1, 1, 10, + PGC_USERSET, + 0, NULL, NULL, NULL); + + MarkGUCPrefixReserved("test_plan_advice"); + + /* Install our planner hook. */ + prev_planner_hook = planner_hook; + planner_hook = test_plan_advice_planner; + /* * Ask pg_plan_advice to get advice strings from test_plan_advice_advisor */ @@ -51,6 +84,78 @@ _PG_init(void) true, NULL); (*add_advisor_fn) (test_plan_advice_advisor); + + /* + * Get a pointer to pg_plan_advice's function for emitting feedback + * warnings. + */ + feedback_warning_fn = + load_external_function("pg_plan_advice", + "pgpa_planner_feedback_warning", + true, NULL); +} + +/* + * Planner hook that retries planning when feedback indicates a problem. + * + * When the catalog changes between the first plan (which generates advice) + * and the second plan (which uses that advice), the advice can reference + * objects that no longer exist or reflect stale statistics. To avoid + * spurious warnings, we retry planning up to test_plan_advice.max_attempts + * times. If the feedback stabilizes (i.e. is the same as the previous + * attempt), we conclude the problem is genuine and emit warnings. + */ +static PlannedStmt * +test_plan_advice_planner(Query *parse, const char *query_string, + int cursorOptions, ParamListInfo boundParams, + ExplainState *es) +{ + PlannedStmt *pstmt; + List *feedback; + List *prev_feedback = NIL; + + for (int i = 0; i < test_plan_advice_max_attempts; ++i) + { + /* + * Try planning the query. On the first iteration, we don't need or + * want to suppress any warnings or other chatter that the planner is + * going to generate, because our goal here is to get the same output + * that would have occurred without this module. But on the second and + * later iterations, that output has already been produced, so we + * don't want it to appear again. + */ + pstmt = copy_and_plan_query(parse, query_string, cursorOptions, + boundParams, es, (i > 0)); + + /* Extract feedback. */ + feedback = extract_feedback(pstmt); + + /* If no problems were detected, stop. */ + if (all_feedback_is_ok(feedback)) + break; + + /* + * If the feedback is the same as last time, then apparently there's + * a real problem, so emit warnings and stop. If this is the last + * iteration, it's less clear that there's a real problem, but if not, + * the user hasn't set the maximum number of retries high enough, so + * handle that case the same way. + */ + if (equal(feedback, prev_feedback) || + i == test_plan_advice_max_attempts - 1) + { + (*feedback_warning_fn) (feedback); + break; + } + + /* + * Go around and try it again, with the newly-generated feedback as + * the new point of comparison. + */ + prev_feedback = feedback; + } + + return pstmt; } /* @@ -63,7 +168,6 @@ test_plan_advice_advisor(PlannerGlobal *glob, Query *parse, ExplainState *es) { PlannedStmt *pstmt; - int save_nestlevel = 0; DefElem *pgpa_item; DefElem *advice_string_item; @@ -74,35 +178,18 @@ test_plan_advice_advisor(PlannerGlobal *glob, Query *parse, if (in_recursion) return NULL; + /* + * Try planning the query, generating advice in the process. We ask + * copy_and_plan_query to adjust client_min_messages; otherwise, any + * messages that are generated during planning would appear here and again + * when the query is replanned with the advice string. + */ PG_TRY(); { in_recursion = true; - /* - * Planning can trigger expression evaluation, which can result in - * sending NOTICE messages or other output to the client. To avoid - * that, we set client_min_messages = ERROR in the hopes of getting - * the same output with and without this module. - * - * We also need to set pg_plan_advice.always_store_advice_details so - * that pg_plan_advice will generate an advice string, since the whole - * point of this function is to get access to that. - */ - save_nestlevel = NewGUCNestLevel(); - set_config_option("client_min_messages", "error", - PGC_SUSET, PGC_S_SESSION, - GUC_ACTION_SAVE, true, 0, false); - set_config_option("pg_plan_advice.always_store_advice_details", "true", - PGC_SUSET, PGC_S_SESSION, - GUC_ACTION_SAVE, true, 0, false); - - /* - * Replan. We must copy the Query, because the planner modifies it. - * (As noted elsewhere, that's unfortunate; perhaps it will be fixed - * some day.) - */ - pstmt = planner(copyObject(parse), query_string, cursorOptions, - glob->boundParams, es); + pstmt = copy_and_plan_query(parse, query_string, cursorOptions, + glob->boundParams, es, true); } PG_FINALLY(); { @@ -110,10 +197,6 @@ test_plan_advice_advisor(PlannerGlobal *glob, Query *parse, } PG_END_TRY(); - /* Roll back any GUC changes */ - if (save_nestlevel > 0) - AtEOXact_GUC(false, save_nestlevel); - /* Extract and return the advice string */ pgpa_item = find_defelem_by_defname(pstmt->extension_state, "pg_plan_advice"); @@ -127,6 +210,96 @@ test_plan_advice_advisor(PlannerGlobal *glob, Query *parse, return strVal(advice_string_item->arg); } +/* + * Wrapper around the main query planner. + */ +static PlannedStmt * +copy_and_plan_query(Query *parse, const char *query_string, int cursorOptions, + ParamListInfo boundParams, ExplainState *es, + bool suppress_messages) +{ + int save_nestlevel = 0; + PlannedStmt *pstmt; + + /* + * Temporarily set pg_plan_advice.always_store_advice_details. Either + * we're being called to generate advice, in which case setting this GUC + * is important to make sure that we do, or we're being called to see + * whether supplied advice applied properly, in which case this is needed + * so that pg_plan_advice will provide feedback. + */ + save_nestlevel = NewGUCNestLevel(); + set_config_option("pg_plan_advice.always_store_advice_details", + "true", + PGC_SUSET, PGC_S_SESSION, + GUC_ACTION_SAVE, true, 0, false); + + /* + * Planning can trigger expression evaluation, which can result in sending + * NOTICE messages or other output to the client. To avoid that, we allow + * the caller to request client_min_messages = ERROR in the hopes of + * getting the same output with and without this module. + */ + if (suppress_messages) + set_config_option("client_min_messages", "error", + PGC_SUSET, PGC_S_SESSION, + GUC_ACTION_SAVE, true, 0, false); + + /* + * We must copy the Query, because the planner modifies it, and we intend + * to plan it multiple times. (As noted elsewhere, that's unfortunate; + * perhaps it will be fixed some day.) + */ + if (prev_planner_hook) + pstmt = (*prev_planner_hook) (copyObject(parse), query_string, + cursorOptions, boundParams, es); + else + pstmt = standard_planner(copyObject(parse), query_string, + cursorOptions, boundParams, es); + + /* Roll back any GUC changes */ + AtEOXact_GUC(false, save_nestlevel); + + /* And we're done. */ + return pstmt; +} + +/* + * Extract the feedback list from a PlannedStmt's extension_state. + * Returns NIL if no feedback is present. + */ +static List * +extract_feedback(PlannedStmt *pstmt) +{ + DefElem *pgpa_item; + DefElem *feedback_item; + + pgpa_item = find_defelem_by_defname(pstmt->extension_state, + "pg_plan_advice"); + if (pgpa_item == NULL) + return NIL; + feedback_item = find_defelem_by_defname((List *) pgpa_item->arg, + "feedback"); + if (feedback_item == NULL) + return NIL; + return (List *) feedback_item->arg; +} + +/* + * Check whether a feedback list indicates that all advice was applied + * successfully. + */ +static bool +all_feedback_is_ok(List *feedback) +{ + foreach_node(DefElem, item, feedback) + { + if (defGetInt32(item) != (PGPA_FB_MATCH_PARTIAL | PGPA_FB_MATCH_FULL)) + return false; + } + return true; +} + /* * Search a list of DefElem objects for a given defname. */ -- 2.51.0