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

Author: michaelpq

expected/pg_hint_plan.out

@@ -9119,6 +9119,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;

pg_hint_plan.c

@@ -3117,43 +3117,15 @@ pg_hint_plan_planner(Query *parse, int cursorOptions, ParamListInfo boundParams)
 
 	/*
 	 * 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
@@ -3165,10 +3137,43 @@ pg_hint_plan_planner(Query *parse, int cursorOptions, ParamListInfo boundParams)
 
 	/*
 	 * 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, cursorOptions, boundParams);
 		else

sql/pg_hint_plan.sql

@@ -1143,6 +1143,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;