From 4a7c037238c9bbee1031060b1dcce8b625eb3f6e Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Thu, 29 Jun 2023 19:06:28 +0900 Subject: [PATCH] Fix hint stack corruption on ERROR when setting GUCs from Set hints The planner hook uses a PG_TRY/PG_CATCH block to handle the hint stack available for a given query. push_hint() is in charge of updating the stack with a new set in the planner hook, and pop_hint() would reset the stack to its previous state. It happens that the code was not cleaning up the stack between push_hint() and the PG_TRY/PG_CATCH block, and that it was possible to trigger ERRORs, corrupting the hint stack hold in a backend. Based on report #141, setting pg_hint_plan.parse_messages TO ERROR with an incorrect Set hint would be one case where the stack would get corrupted, because the GUCs defined in such hints are loaded after the hint stack update and before PG_TRY(). Any errors happening before PG_TRY() would be enough to corrupt the stack, like a palloc() failing on OOM, though this is very unlikely. This commit takes care of this bug by removing completely the window where the stack could get corrupted, by moving PG_TRY a little bit earlier. A regression test is added, provided by reporter. The patch is from me. Per report #141, from Egor Chindyaskin. Backpatch-through: 11 --- expected/pg_hint_plan.out | 16 +++++++++ pg_hint_plan.c | 69 +++++++++++++++++++++------------------ sql/pg_hint_plan.sql | 4 +++ 3 files changed, 57 insertions(+), 32 deletions(-) diff --git a/expected/pg_hint_plan.out b/expected/pg_hint_plan.out index 76a426f9..aca2dba1 100644 --- a/expected/pg_hint_plan.out +++ b/expected/pg_hint_plan.out @@ -9129,6 +9129,22 @@ error hint: ----+----- (0 rows) +set pg_hint_plan.parse_messages to 'ERROR'; +-- Force an error before running the planner hook, when forcing the Set hints. +/*+ Set(work_mem "foo") */ SELECT 1; +ERROR: invalid value for parameter "work_mem": "foo" +/*+ SeqScan(t1) */ SELECT * FROM t1 LIMIT 0; +LOG: pg_hint_plan: +used hint: +SeqScan(t1) +not used hint: +duplication hint: +error hint: + + id | val +----+----- +(0 rows) + set pg_hint_plan.message_level to 'DEBUG1'; set pg_hint_plan.parse_messages to 'NOTICE'; /*+ SeqScan( */ SELECT 1; diff --git a/pg_hint_plan.c b/pg_hint_plan.c index 718f2db0..717598f6 100644 --- a/pg_hint_plan.c +++ b/pg_hint_plan.c @@ -3065,43 +3065,15 @@ pg_hint_plan_planner(Query *parse, const char *query_string, int cursorOptions, /* * Push new hint struct to the hint stack to disable previous hint context. + * There should be no ERROR-level failures until we begin the + * PG_TRY/PG_CATCH block below to ensure a consistent stack handling all + * the time. */ push_hint(hstate); /* Set scan enforcement here. */ save_nestlevel = NewGUCNestLevel(); - /* Apply Set hints, then save it as the initial state */ - setup_guc_enforcement(current_hint_state->set_hints, - current_hint_state->num_hints[HINT_TYPE_SET], - current_hint_state->context); - - current_hint_state->init_scan_mask = get_current_scan_mask(); - current_hint_state->init_join_mask = get_current_join_mask(); - current_hint_state->init_min_para_tablescan_size = - min_parallel_table_scan_size; - current_hint_state->init_min_para_indexscan_size = - min_parallel_index_scan_size; - current_hint_state->init_paratup_cost = parallel_tuple_cost; - current_hint_state->init_parasetup_cost = parallel_setup_cost; - - /* - * max_parallel_workers_per_gather should be non-zero here if Workers hint - * is specified. - */ - if (max_hint_nworkers > 0 && max_parallel_workers_per_gather < 1) - set_config_int32_option("max_parallel_workers_per_gather", - 1, current_hint_state->context); - current_hint_state->init_nworkers = max_parallel_workers_per_gather; - - if (debug_level > 1) - { - ereport(pg_hint_plan_debug_message_level, - (errhidestmt(msgqno != qno), - errmsg("pg_hint_plan%s: planner", qnostr))); - msgqno = qno; - } - /* * The planner call below may replace current_hint_str. Store and restore * it so that the subsequent planning in the upper level doesn't get @@ -3113,10 +3085,43 @@ pg_hint_plan_planner(Query *parse, const char *query_string, int cursorOptions, /* * Use PG_TRY mechanism to recover GUC parameters and current_hint_state to - * the state when this planner started when error occurred in planner. + * the state when this planner started when error occurred in planner. We + * do this here to minimize the window where the hints currently pushed on + * the stack could not be popped out of it. */ PG_TRY(); { + /* Apply Set hints, then save it as the initial state */ + setup_guc_enforcement(current_hint_state->set_hints, + current_hint_state->num_hints[HINT_TYPE_SET], + current_hint_state->context); + + current_hint_state->init_scan_mask = get_current_scan_mask(); + current_hint_state->init_join_mask = get_current_join_mask(); + current_hint_state->init_min_para_tablescan_size = + min_parallel_table_scan_size; + current_hint_state->init_min_para_indexscan_size = + min_parallel_index_scan_size; + current_hint_state->init_paratup_cost = parallel_tuple_cost; + current_hint_state->init_parasetup_cost = parallel_setup_cost; + + /* + * max_parallel_workers_per_gather should be non-zero here if Workers + * hint is specified. + */ + if (max_hint_nworkers > 0 && max_parallel_workers_per_gather < 1) + set_config_int32_option("max_parallel_workers_per_gather", + 1, current_hint_state->context); + current_hint_state->init_nworkers = max_parallel_workers_per_gather; + + if (debug_level > 1) + { + ereport(pg_hint_plan_debug_message_level, + (errhidestmt(msgqno != qno), + errmsg("pg_hint_plan%s: planner", qnostr))); + msgqno = qno; + } + if (prev_planner) result = (*prev_planner) (parse, query_string, cursorOptions, boundParams); diff --git a/sql/pg_hint_plan.sql b/sql/pg_hint_plan.sql index 135248f3..18023f06 100644 --- a/sql/pg_hint_plan.sql +++ b/sql/pg_hint_plan.sql @@ -1156,6 +1156,10 @@ set client_min_messages to 'DEBUG1'; set pg_hint_plan.debug_level to 'verbose'; /*+ SeqScan( */ SELECT 1; /*+ SeqScan(t1) */ SELECT * FROM t1 LIMIT 0; +set pg_hint_plan.parse_messages to 'ERROR'; +-- Force an error before running the planner hook, when forcing the Set hints. +/*+ Set(work_mem "foo") */ SELECT 1; +/*+ SeqScan(t1) */ SELECT * FROM t1 LIMIT 0; set pg_hint_plan.message_level to 'DEBUG1'; set pg_hint_plan.parse_messages to 'NOTICE'; /*+ SeqScan( */ SELECT 1;