nocfbot-0001-pg_plan_advice-Various-fixes.patch

From 25d299dfe5761e2354af12c856e7d7cb1dc3333d Mon Sep 17 00:00:00 2001
From: Lukas Fittl <lukas@fittl.com>
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