From 25d299dfe5761e2354af12c856e7d7cb1dc3333d Mon Sep 17 00:00:00 2001 From: Lukas Fittl Date: Thu, 12 Mar 2026 17:34:24 -0700 Subject: [PATCH 1/2] pg_plan_advice: Various fixes - Free nested join data in pgpa_destroy_join_unroller - Add explicit YYABORT calls when parser encounters errors - Use pg_strtoint32_safe for parsing decinteger to correctly handle underscores - Don't use "0" for setting a boolean value in pgpa_walk_recursively - Allow partition and plan names to use advice names (e.g. "a@hash_join"), this is needed since we don't quote them when emitting --- contrib/pg_plan_advice/expected/syntax.out | 45 ++++++++++++++++++++++ contrib/pg_plan_advice/pgpa_join.c | 5 +++ contrib/pg_plan_advice/pgpa_parser.y | 20 +++++++--- contrib/pg_plan_advice/pgpa_scanner.l | 10 ++--- contrib/pg_plan_advice/pgpa_walker.c | 2 +- contrib/pg_plan_advice/sql/syntax.sql | 19 +++++++++ 6 files changed, 90 insertions(+), 11 deletions(-) diff --git a/contrib/pg_plan_advice/expected/syntax.out b/contrib/pg_plan_advice/expected/syntax.out index be61402b569..c3f2cbd6dca 100644 --- a/contrib/pg_plan_advice/expected/syntax.out +++ b/contrib/pg_plan_advice/expected/syntax.out @@ -190,3 +190,48 @@ DETAIL: Could not parse advice: FOREIGN_JOIN targets must contain more than one SET pg_plan_advice.advice = 'FOREIGN_JOIN((a))'; ERROR: invalid value for parameter "pg_plan_advice.advice": "FOREIGN_JOIN((a))" DETAIL: Could not parse advice: FOREIGN_JOIN targets must contain more than one relation identifier at or near ")" +-- Tag keywords used as alias names work fine, because the 'identifier' +-- nonterminal accepts all token types. +SET pg_plan_advice.advice = 'SEQ_SCAN(hash_join)'; +EXPLAIN (COSTS OFF) SELECT 1; + QUERY PLAN +----------------------------------------- + Result + Supplied Plan Advice: + SEQ_SCAN(hash_join) /* not matched */ +(3 rows) + +SET pg_plan_advice.advice = 'SEQ_SCAN(seq_scan)'; +EXPLAIN (COSTS OFF) SELECT 1; + QUERY PLAN +---------------------------------------- + Result + Supplied Plan Advice: + SEQ_SCAN(seq_scan) /* not matched */ +(3 rows) + +SET pg_plan_advice.advice = 'SEQ_SCAN(gather)'; +EXPLAIN (COSTS OFF) SELECT 1; + QUERY PLAN +-------------------------------------- + Result + Supplied Plan Advice: + SEQ_SCAN(gather) /* not matched */ +(3 rows) + +SET pg_plan_advice.advice = 'SEQ_SCAN(join_order)'; +EXPLAIN (COSTS OFF) SELECT 1; + QUERY PLAN +------------------------------------------ + Result + Supplied Plan Advice: + SEQ_SCAN(join_order) /* not matched */ +(3 rows) + +-- Tag keywords used as partition names or plan names should also work, +-- since pgpa_identifier_string() can generate these from real partition +-- and subquery names. +SET pg_plan_advice.advice = 'SEQ_SCAN(t/public.hash_join)'; +SET pg_plan_advice.advice = 'SEQ_SCAN(t/hash_join.foo)'; +SET pg_plan_advice.advice = 'SEQ_SCAN(t@hash_join)'; +SET pg_plan_advice.advice = 'SEQ_SCAN(t@seq_scan)'; diff --git a/contrib/pg_plan_advice/pgpa_join.c b/contrib/pg_plan_advice/pgpa_join.c index 4610d02356f..02f7fdedbe3 100644 --- a/contrib/pg_plan_advice/pgpa_join.c +++ b/contrib/pg_plan_advice/pgpa_join.c @@ -294,6 +294,11 @@ pgpa_build_unrolled_join(pgpa_plan_walker_context *walker, void pgpa_destroy_join_unroller(pgpa_join_unroller *join_unroller) { + for (unsigned i = 0; i < join_unroller->nused; i++) + { + if (join_unroller->inner_unrollers[i] != NULL) + pgpa_destroy_join_unroller(join_unroller->inner_unrollers[i]); + } pfree(join_unroller->strategy); pfree(join_unroller->inner_subplans); pfree(join_unroller->inner_elided_nodes); diff --git a/contrib/pg_plan_advice/pgpa_parser.y b/contrib/pg_plan_advice/pgpa_parser.y index 8bfd7666d07..4ec4a3bdfac 100644 --- a/contrib/pg_plan_advice/pgpa_parser.y +++ b/contrib/pg_plan_advice/pgpa_parser.y @@ -87,8 +87,11 @@ advice_item: TOK_TAG_JOIN_ORDER '(' join_order_target_list ')' $$->tag = PGPA_TAG_JOIN_ORDER; $$->targets = $3; if ($3 == NIL) + { pgpa_yyerror(result, parse_error_msg_p, yyscanner, "JOIN_ORDER must have at least one target"); + YYABORT; + } } | TOK_TAG_INDEX '(' index_target_list ')' { @@ -126,6 +129,7 @@ advice_item: TOK_TAG_JOIN_ORDER '(' join_order_target_list ')' { pgpa_yyerror(result, parse_error_msg_p, yyscanner, "unrecognized advice tag"); + YYABORT; } if ($$->tag == PGPA_TAG_FOREIGN_JOIN) @@ -134,8 +138,11 @@ advice_item: TOK_TAG_JOIN_ORDER '(' join_order_target_list ')' { if (target->ttype == PGPA_TARGET_IDENTIFIER || list_length(target->children) == 1) - pgpa_yyerror(result, parse_error_msg_p, yyscanner, - "FOREIGN_JOIN targets must contain more than one relation identifier"); + { + pgpa_yyerror(result, parse_error_msg_p, yyscanner, + "FOREIGN_JOIN targets must contain more than one relation identifier"); + YYABORT; + } } } @@ -177,8 +184,11 @@ opt_ri_occurrence: '#' TOK_INTEGER { if ($2 <= 0) + { pgpa_yyerror(result, parse_error_msg_p, yyscanner, "only positive occurrence numbers are permitted"); + YYABORT; + } $$ = $2; } | @@ -200,16 +210,16 @@ identifier: TOK_IDENT * when parsing advice, we accept a specification that lacks one. */ opt_partition: - '/' TOK_IDENT '.' TOK_IDENT + '/' identifier '.' identifier { $$ = list_make2($2, $4); } - | '/' TOK_IDENT + | '/' identifier { $$ = list_make1($2); } | { $$ = NIL; } ; opt_plan_name: - '@' TOK_IDENT + '@' identifier { $$ = $2; } | { $$ = NULL; } diff --git a/contrib/pg_plan_advice/pgpa_scanner.l b/contrib/pg_plan_advice/pgpa_scanner.l index 3b3be6eb727..a04c3be3e72 100644 --- a/contrib/pg_plan_advice/pgpa_scanner.l +++ b/contrib/pg_plan_advice/pgpa_scanner.l @@ -8,9 +8,9 @@ */ #include "postgres.h" -#include "common/string.h" #include "nodes/miscnodes.h" #include "parser/scansup.h" +#include "utils/builtins.h" #include "pgpa_ast.h" #include "pgpa_parser.h" @@ -135,11 +135,11 @@ xcinside [^*/]+ } {decinteger} { - char *endptr; + ErrorSaveContext escontext = {T_ErrorSaveContext}; - errno = 0; - yylval->integer = strtoint(yytext, &endptr, 10); - if (*endptr != '\0' || errno == ERANGE) + yylval->integer = + pg_strtoint32_safe(yytext, (Node *) &escontext); + if (escontext.error_occurred) pgpa_yyerror(result, parse_error_msg_p, yyscanner, "integer out of range"); return TOK_INTEGER; diff --git a/contrib/pg_plan_advice/pgpa_walker.c b/contrib/pg_plan_advice/pgpa_walker.c index 7b86cc5e6f9..c1203f123a5 100644 --- a/contrib/pg_plan_advice/pgpa_walker.c +++ b/contrib/pg_plan_advice/pgpa_walker.c @@ -432,7 +432,7 @@ pgpa_walk_recursively(pgpa_plan_walker_context *walker, Plan *plan, * those are specific to a subquery level. */ pgpa_walk_recursively(walker, ((SubqueryScan *) plan)->subplan, - 0, NULL, NIL, beneath_any_gather); + false, NULL, NIL, beneath_any_gather); break; case T_CustomScan: extraplans = ((CustomScan *) plan)->custom_plans; diff --git a/contrib/pg_plan_advice/sql/syntax.sql b/contrib/pg_plan_advice/sql/syntax.sql index 56a5d54e2b5..f274fa48636 100644 --- a/contrib/pg_plan_advice/sql/syntax.sql +++ b/contrib/pg_plan_advice/sql/syntax.sql @@ -66,3 +66,22 @@ SET pg_plan_advice.advice = '/*/* stuff */*/'; -- Foreign join requires multiple relation identifiers. SET pg_plan_advice.advice = 'FOREIGN_JOIN(a)'; SET pg_plan_advice.advice = 'FOREIGN_JOIN((a))'; + +-- Tag keywords used as alias names work fine, because the 'identifier' +-- nonterminal accepts all token types. +SET pg_plan_advice.advice = 'SEQ_SCAN(hash_join)'; +EXPLAIN (COSTS OFF) SELECT 1; +SET pg_plan_advice.advice = 'SEQ_SCAN(seq_scan)'; +EXPLAIN (COSTS OFF) SELECT 1; +SET pg_plan_advice.advice = 'SEQ_SCAN(gather)'; +EXPLAIN (COSTS OFF) SELECT 1; +SET pg_plan_advice.advice = 'SEQ_SCAN(join_order)'; +EXPLAIN (COSTS OFF) SELECT 1; + +-- Tag keywords used as partition names or plan names should also work, +-- since pgpa_identifier_string() can generate these from real partition +-- and subquery names. +SET pg_plan_advice.advice = 'SEQ_SCAN(t/public.hash_join)'; +SET pg_plan_advice.advice = 'SEQ_SCAN(t/hash_join.foo)'; +SET pg_plan_advice.advice = 'SEQ_SCAN(t@hash_join)'; +SET pg_plan_advice.advice = 'SEQ_SCAN(t@seq_scan)'; -- 2.47.1