From cdc67c04a1469366b560122aeba996458af533c5 Mon Sep 17 00:00:00 2001 From: Robert Haas Date: Fri, 16 Oct 2026 09:23:00 -0400 Subject: [PATCH v8 1/3] pg_plan_advice: Fix multiple problems with NO_GATHER advice. First, the documentation describes NO_GATHER in a way that suggests that it should both suppress partial path generation and also prohibit the placement of Gather and Gather Merge within a join problem, but in fact it only did the latter. Change it to do the former as well. Like commit f71a9969d58e3e70239fc51bbeae668db973b265, this is fallout from my failure to realize everything that needed to be changed when I abandoned the idea of clearing consider_parallel as a way to suppress parallel paths. Second, emit NO_GATHER advice for the non-partial children of a Parallel Append, because we want exactly the behaviors described above for that case. In order to reproduce the Parallel Append, we'll need an entirely non-parallel path for that child. Third, update the documentation to be more precise about what happens when NO_GATHER is applied to a child table. I believe I intended this to work in the way described in the first paragraph above, but because of the omission noted there, it had no effect at all. Moreover, the documentation described the two different effects of NO_GATHER as if subqueries were the only relevant case, but partitioning or table inheritance gives rise to similar effects, so clarify accordingly. --- contrib/pg_plan_advice/expected/gather.out | 129 +++++++++++++++++++++ contrib/pg_plan_advice/pgpa_planner.c | 101 ++++++++++++++++ contrib/pg_plan_advice/pgpa_walker.c | 20 +++- contrib/pg_plan_advice/sql/gather.sql | 37 ++++++ doc/src/sgml/pgplanadvice.sgml | 13 ++- src/tools/pgindent/typedefs.list | 1 + 6 files changed, 297 insertions(+), 4 deletions(-) diff --git a/contrib/pg_plan_advice/expected/gather.out b/contrib/pg_plan_advice/expected/gather.out index 0650381479a..85e13a2dcc5 100644 --- a/contrib/pg_plan_advice/expected/gather.out +++ b/contrib/pg_plan_advice/expected/gather.out @@ -435,3 +435,132 @@ EXPLAIN (COSTS OFF, PLAN_ADVICE) (8 rows) COMMIT; +-- Test interaction of NO_GATHER with UNION. By default, we expect a +-- Gather over a Parallel Append, but no_gather on the setop arms is +-- enough to suppress it. +BEGIN; +EXPLAIN (COSTS OFF, PLAN_ADVICE) + SELECT dim_id FROM gt_fact UNION SELECT dim_id FROM gt_fact; + QUERY PLAN +---------------------------------------------------------- + HashAggregate + Group Key: gt_fact.dim_id + -> Gather + Workers Planned: 1 + -> Parallel Append + -> Parallel Seq Scan on gt_fact + -> Parallel Seq Scan on gt_fact gt_fact_1 + Generated Plan Advice: + SEQ_SCAN(gt_fact@setop_1 gt_fact@setop_2) + GATHER((unnamed_subquery unnamed_subquery#2)) +(10 rows) + +SET LOCAL pg_plan_advice.advice = 'NO_GATHER(gt_fact@setop_1 gt_fact@setop_2)'; +EXPLAIN (COSTS OFF, PLAN_ADVICE) + SELECT dim_id FROM gt_fact UNION SELECT dim_id FROM gt_fact; + QUERY PLAN +---------------------------------------------------------------------------------- + HashAggregate + Group Key: gt_fact.dim_id + -> Append + -> Seq Scan on gt_fact + -> Seq Scan on gt_fact gt_fact_1 + Supplied Plan Advice: + NO_GATHER(gt_fact@setop_1) /* matched */ + NO_GATHER(gt_fact@setop_2) /* matched */ + Generated Plan Advice: + SEQ_SCAN(gt_fact@setop_1 gt_fact@setop_2) + NO_GATHER(unnamed_subquery unnamed_subquery#2 gt_fact@setop_1 gt_fact@setop_2) +(11 rows) + +COMMIT; +-- Test interaction of NO_GATHER with a UNION that has just one surviving +-- arm. In this case, the Append node is elided. +BEGIN; +EXPLAIN (COSTS OFF, PLAN_ADVICE) + SELECT dim_id FROM gt_fact UNION SELECT dim_id FROM gt_fact WHERE false; + QUERY PLAN +-------------------------------------------------- + HashAggregate + Group Key: gt_fact.dim_id + -> Gather + Workers Planned: 1 + -> Parallel Seq Scan on gt_fact + Generated Plan Advice: + SEQ_SCAN(gt_fact@setop_1) + GATHER(gt_fact@setop_1) + NO_GATHER(unnamed_subquery unnamed_subquery#2) +(9 rows) + +SET LOCAL pg_plan_advice.advice = 'NO_GATHER(gt_fact@setop_1)'; +EXPLAIN (COSTS OFF, PLAN_ADVICE) + SELECT dim_id FROM gt_fact UNION SELECT dim_id FROM gt_fact WHERE false; + QUERY PLAN +------------------------------------------------------------------ + HashAggregate + Group Key: gt_fact.dim_id + -> Seq Scan on gt_fact + Supplied Plan Advice: + NO_GATHER(gt_fact@setop_1) /* matched */ + Generated Plan Advice: + SEQ_SCAN(gt_fact@setop_1) + NO_GATHER(unnamed_subquery unnamed_subquery#2 gt_fact@setop_1) +(8 rows) + +COMMIT; +-- Test interaction of NO_GATHER with a UNION ALL. This UNION ALL is +-- "flattened" by the planner and the subqueries are pulled up, so the relation +-- identifiers are different from the preceding examples. In this case, +-- we can ask for either a Parallel Append with non-partial children, or +-- suppress Gather and Gather Merge completely. +BEGIN; +EXPLAIN (COSTS OFF, PLAN_ADVICE) + SELECT dim_id FROM gt_fact UNION ALL SELECT dim_id FROM gt_fact; + QUERY PLAN +---------------------------------------------------- + Gather + Workers Planned: 1 + -> Parallel Append + -> Parallel Seq Scan on gt_fact + -> Parallel Seq Scan on gt_fact gt_fact_1 + Generated Plan Advice: + SEQ_SCAN(gt_fact gt_fact#2) + GATHER(unnamed_subquery) +(8 rows) + +SET LOCAL pg_plan_advice.advice = 'GATHER(unnamed_subquery) NO_GATHER(gt_fact gt_fact#2)'; +EXPLAIN (COSTS OFF, PLAN_ADVICE) + SELECT dim_id FROM gt_fact UNION ALL SELECT dim_id FROM gt_fact; + QUERY PLAN +------------------------------------------- + Gather + Workers Planned: 1 + -> Parallel Append + -> Seq Scan on gt_fact + -> Seq Scan on gt_fact gt_fact_1 + Supplied Plan Advice: + GATHER(unnamed_subquery) /* matched */ + NO_GATHER(gt_fact) /* matched */ + NO_GATHER(gt_fact#2) /* matched */ + Generated Plan Advice: + SEQ_SCAN(gt_fact gt_fact#2) + GATHER(unnamed_subquery) + NO_GATHER(gt_fact gt_fact#2) +(13 rows) + +SET LOCAL pg_plan_advice.advice = 'NO_GATHER(unnamed_subquery)'; +EXPLAIN (COSTS OFF, PLAN_ADVICE) + SELECT dim_id FROM gt_fact UNION ALL SELECT dim_id FROM gt_fact; + QUERY PLAN +------------------------------------------------- + Append + -> Seq Scan on gt_fact + -> Seq Scan on gt_fact gt_fact_1 + Supplied Plan Advice: + NO_GATHER(unnamed_subquery) /* matched */ + Generated Plan Advice: + SEQ_SCAN(gt_fact gt_fact#2) + NO_GATHER(unnamed_subquery gt_fact gt_fact#2) +(8 rows) + +COMMIT; diff --git a/contrib/pg_plan_advice/pgpa_planner.c b/contrib/pg_plan_advice/pgpa_planner.c index 9f05fa033ba..6551037e796 100644 --- a/contrib/pg_plan_advice/pgpa_planner.c +++ b/contrib/pg_plan_advice/pgpa_planner.c @@ -87,8 +87,15 @@ typedef struct pgpa_join_state Bitmapset *rel_indexes; } pgpa_join_state; +typedef struct pgpa_baserel_state +{ + /* Force partial paths to be discarded? */ + bool discard_partial_paths; +} pgpa_baserel_state; + /* Saved hook values */ static build_simple_rel_hook_type prev_build_simple_rel = NULL; +static set_rel_pathlist_hook_type prev_set_rel_pathlist = NULL; static join_path_setup_hook_type prev_join_path_setup = NULL; static joinrel_setup_hook_type prev_joinrel_setup = NULL; static planner_setup_hook_type prev_planner_setup = NULL; @@ -109,6 +116,10 @@ static void pgpa_planner_shutdown(PlannerGlobal *glob, Query *parse, static void pgpa_build_simple_rel(PlannerInfo *root, RelOptInfo *rel, RangeTblEntry *rte); +static void pgpa_set_rel_pathlist(PlannerInfo *root, + RelOptInfo *rel, + Index rti, + RangeTblEntry *rte); static void pgpa_joinrel_setup(PlannerInfo *root, RelOptInfo *joinrel, RelOptInfo *outerrel, @@ -188,6 +199,8 @@ pgpa_planner_install_hooks(void) planner_shutdown_hook = pgpa_planner_shutdown; prev_build_simple_rel = build_simple_rel_hook; build_simple_rel_hook = pgpa_build_simple_rel; + prev_set_rel_pathlist = set_rel_pathlist_hook; + set_rel_pathlist_hook = pgpa_set_rel_pathlist; prev_joinrel_setup = joinrel_setup_hook; joinrel_setup_hook = pgpa_joinrel_setup; prev_join_path_setup = join_path_setup_hook; @@ -467,6 +480,28 @@ pgpa_build_simple_rel(PlannerInfo *root, RelOptInfo *rel, RangeTblEntry *rte) (*prev_build_simple_rel) (root, rel, rte); } +/* + * Hook function for set_rel_pathlist(). + */ +static void +pgpa_set_rel_pathlist(PlannerInfo *root, RelOptInfo *rel, Index rti, + RangeTblEntry *rte) +{ + pgpa_baserel_state *pbs; + + if (prev_set_rel_pathlist) + (*prev_set_rel_pathlist) (root, rel, rti, rte); + + /* + * Discard any partial paths, if we were asked to do so. We do this after + * calling any previous hook, so that we can discard whatever paths it may + * have added in addition to whatever came from the core code. + */ + pbs = GetRelOptInfoExtensionState(rel, planner_extension_id); + if (pbs != NULL && pbs->discard_partial_paths) + rel->partial_pathlist = NIL; +} + /* * Enforce any provided advice that is relevant to any method of implementing * this join. @@ -1652,6 +1687,8 @@ pgpa_planner_apply_scan_advice(RelOptInfo *rel, const uint64 all_scan_mask = PGS_SCAN_ANY | PGS_APPEND | PGS_MERGE_APPEND | PGS_CONSIDER_INDEXONLY; bool gather_conflict = false; + bool saw_no_gather = false; + bool saw_gather = false; Bitmapset *gather_partial_match = NULL; Bitmapset *gather_full_match = NULL; int i = -1; @@ -1777,11 +1814,13 @@ pgpa_planner_apply_scan_advice(RelOptInfo *rel, my_gather_mask = PGS_GATHER; else my_gather_mask = PGS_GATHER_MERGE; + saw_gather = true; } else if (my_entry->tag == PGPA_TAG_NO_GATHER) { Assert(just_one_rel); my_gather_mask = PGS_CONSIDER_NONPARTIAL; + saw_no_gather = true; } /* @@ -1844,6 +1883,68 @@ pgpa_planner_apply_scan_advice(RelOptInfo *rel, } } + /* + * NO_GATHER is documented to both discard partial paths for the current + * relation and also to disable placement of a Gather or Gather Merge node + * above a scan or join containing this target. + * + * These may sound like the same thing, but they aren't. Preventing a + * Gather or Gather Merge node above a scan or join containing this target + * only disables the use of parallel query within the same join problem. + * If that were the only thing we prohibited, then a scan of a parent + * table, or an outer subquery, would potentially still be able to use a + * Parallel Seq Scan or similar on this table. + * + * Since that's counterintuitive, we also arrange to discard all partial + * paths for this relation. That doesn't totally block parallel query, + * either. An especially notable case is that a non-partial path can be + * joined to a partial path to produce a new partial path. + * + * Even taken together, these are *still* not a total prohibition. An + * outer subquery won't be able to scan this relation in parallel, but it + * can still use a non-partial scan of this relation at some point in the + * plan tree that is below a Gather or Gather Merge. Also, NO_GATHER on a + * child blocks a parallel scan of that child, but a parent could still + * create a Parallel Append node that incorporates a non-partial scan of + * that child. + * + * Possibly the whole definition of NO_GATHER needs rethinking in the + * future, but it needs careful thought, because we must be able to + * distinguish between closely-related cases; for example, it's easy to + * argue that NO_GATHER on a subquery rel should prohibit parallelism + * entirely at outer levels, but advice is supposed to be able to describe + * every plan shape, and that definition would make it impossible to + * request that the subquery plan avoid parallel scans while at the same + * time demanding a Gather or Gather Merge in the outer query. + * + * Before changing anything here, please review the documentation, which + * discusses the current behavior in some detail. + */ + if (saw_no_gather) + { + if (saw_gather) + { + /* + * GATHER((this other)) and NO_GATHER(this) produce the same + * gather_mask; in such cases, only this code notices the + * conflict. + */ + gather_conflict = true; + } + else + { + pgpa_baserel_state *pbs; + + /* + * Let pgpa_set_rel_pathlist know that it should discard partial + * paths for this relation. + */ + pbs = palloc_object(pgpa_baserel_state); + pbs->discard_partial_paths = true; + SetRelOptInfoExtensionState(rel, planner_extension_id, pbs); + } + } + /* * Mark all the scan method entries as fully matched; and if they specify * different things, mark them all as conflicting. diff --git a/contrib/pg_plan_advice/pgpa_walker.c b/contrib/pg_plan_advice/pgpa_walker.c index bfd1a8e3be1..e415f880d21 100644 --- a/contrib/pg_plan_advice/pgpa_walker.c +++ b/contrib/pg_plan_advice/pgpa_walker.c @@ -242,7 +242,8 @@ pgpa_plan_walker(pgpa_plan_walker_context *walker, PlannedStmt *pstmt, * during tree traversal to each of these query features. * * If beneath_any_gather == true, some higher level of the tree traversal found - * a Gather or Gather Merge node. + * a Gather or Gather Merge node, except that we reset it for the non-partial + * children of a Parallel Append, for reasons explained in the comments below. */ static void pgpa_walk_recursively(pgpa_plan_walker_context *walker, Plan *plan, @@ -257,6 +258,7 @@ pgpa_walk_recursively(pgpa_plan_walker_context *walker, Plan *plan, ListCell *lc; List *extraplans = NIL; List *elided_nodes = NIL; + int first_partial_plan = 0; Assert(within_join_problem || join_unroller == NULL); @@ -444,6 +446,8 @@ pgpa_walk_recursively(pgpa_plan_walker_context *walker, Plan *plan, Append *aplan = (Append *) plan; extraplans = aplan->appendplans; + if (aplan->plan.parallel_aware) + first_partial_plan = aplan->first_partial_plan; } break; case T_MergeAppend: @@ -479,9 +483,21 @@ pgpa_walk_recursively(pgpa_plan_walker_context *walker, Plan *plan, foreach(lc, extraplans) { Plan *subplan = lfirst(lc); + bool child_beneath_any_gather = beneath_any_gather; + + /* + * NO_GATHER() is defined to both suppress placement of Gather and + * Gather Merge nodes within a join problem, and also to suppress + * generation of partial paths. For a non-partial child of a Parallel + * Append, both behaviors are appropriate, even though such a plan + * node is necessarily beneath a Gather or Gather Merge node. Hence, + * we fudge. + */ + if (foreach_current_index(lc) < first_partial_plan) + child_beneath_any_gather = false; pgpa_walk_recursively(walker, subplan, false, NULL, NIL, - beneath_any_gather); + child_beneath_any_gather); } } diff --git a/contrib/pg_plan_advice/sql/gather.sql b/contrib/pg_plan_advice/sql/gather.sql index 616be396c5b..f01850285db 100644 --- a/contrib/pg_plan_advice/sql/gather.sql +++ b/contrib/pg_plan_advice/sql/gather.sql @@ -110,3 +110,40 @@ EXPLAIN (COSTS OFF, PLAN_ADVICE) EXPLAIN (COSTS OFF, PLAN_ADVICE) SELECT * FROM gt_part WHERE id < 500 AND val = 42; COMMIT; + +-- Test interaction of NO_GATHER with UNION. By default, we expect a +-- Gather over a Parallel Append, but no_gather on the setop arms is +-- enough to suppress it. +BEGIN; +EXPLAIN (COSTS OFF, PLAN_ADVICE) + SELECT dim_id FROM gt_fact UNION SELECT dim_id FROM gt_fact; +SET LOCAL pg_plan_advice.advice = 'NO_GATHER(gt_fact@setop_1 gt_fact@setop_2)'; +EXPLAIN (COSTS OFF, PLAN_ADVICE) + SELECT dim_id FROM gt_fact UNION SELECT dim_id FROM gt_fact; +COMMIT; + +-- Test interaction of NO_GATHER with a UNION that has just one surviving +-- arm. In this case, the Append node is elided. +BEGIN; +EXPLAIN (COSTS OFF, PLAN_ADVICE) + SELECT dim_id FROM gt_fact UNION SELECT dim_id FROM gt_fact WHERE false; +SET LOCAL pg_plan_advice.advice = 'NO_GATHER(gt_fact@setop_1)'; +EXPLAIN (COSTS OFF, PLAN_ADVICE) + SELECT dim_id FROM gt_fact UNION SELECT dim_id FROM gt_fact WHERE false; +COMMIT; + +-- Test interaction of NO_GATHER with a UNION ALL. This UNION ALL is +-- "flattened" by the planner and the subqueries are pulled up, so the relation +-- identifiers are different from the preceding examples. In this case, +-- we can ask for either a Parallel Append with non-partial children, or +-- suppress Gather and Gather Merge completely. +BEGIN; +EXPLAIN (COSTS OFF, PLAN_ADVICE) + SELECT dim_id FROM gt_fact UNION ALL SELECT dim_id FROM gt_fact; +SET LOCAL pg_plan_advice.advice = 'GATHER(unnamed_subquery) NO_GATHER(gt_fact gt_fact#2)'; +EXPLAIN (COSTS OFF, PLAN_ADVICE) + SELECT dim_id FROM gt_fact UNION ALL SELECT dim_id FROM gt_fact; +SET LOCAL pg_plan_advice.advice = 'NO_GATHER(unnamed_subquery)'; +EXPLAIN (COSTS OFF, PLAN_ADVICE) + SELECT dim_id FROM gt_fact UNION ALL SELECT dim_id FROM gt_fact; +COMMIT; diff --git a/doc/src/sgml/pgplanadvice.sgml b/doc/src/sgml/pgplanadvice.sgml index a618cb4a4bb..34c3c431b19 100644 --- a/doc/src/sgml/pgplanadvice.sgml +++ b/doc/src/sgml/pgplanadvice.sgml @@ -503,8 +503,9 @@ NO_GATHER(advice_target [ ... ]) NO_GATHER specifies that no Gather or Gather Merge node should appear above any of the - targets, but it only constrains the planning of an individual subquery, - and outer subquery levels can still use parallel query. For example, + targets, but it only constrains planning within a given join problem, + and parent tables or outer subquery levels can still use parallel query. + For example, NO_GATHER(inner_example@any_1) precludes using a Parallel Seq Scan to access the inner_example table within the any_1 @@ -542,6 +543,14 @@ Aggregate Filter: (something_else > 100) + + Similarly, when NO_GATHER is applied to an individual + partition, it precludes a parallel scan of that particular partition, but + it is still possible for a Parallel Append node to use + a non-parallel scan of that partition. To rule out that possibility, + use NO_GATHER on the parent rather than on the child. + + diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index a7a8dbdd23f..ccbfef82d52 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -4079,6 +4079,7 @@ pgp_armor_headers_state pgpa_advice_item pgpa_advice_tag_type pgpa_advice_target +pgpa_baserel_state pgpa_identifier pgpa_index_target pgpa_itm_type -- 2.53.0