From cfcb259e83bba6f8de4e7e5f7580b209b71b5312 Mon Sep 17 00:00:00 2001 From: Robert Haas Date: Mon, 5 Oct 2026 11:42:16 -0400 Subject: [PATCH v7 5/5] pg_plan_advice: Fix advice feedback JOIN_ORDER over a single sublist. Suppose that a JOIN_ORDER() advice target consists of a single ordered or unordered sublist, something like JOIN_ORDER((...)) or JOIN_ORDER({...}) or even JOIN_ORDER(({...})). In the previous coding, the feedback for such advice would be "partially matched" even when the query joined all the relations mentioned. Fix that. Like the issues fixed by e1d8f81d4961ca6c4228eb9a3ab5d36168008e42, this escaped detection because advice generation never creates advice strings that have the problematic form. Reported-by: Ayush Tiwari Discussion: http://postgr.es/m/CAJTYsWWe8E8ya-24wRws5LSVhHNxCwLyy8c-ueY-TKvC=8P7UQ@mail.gmail.com Backpatch-through: 19 --- .../pg_plan_advice/expected/join_order.out | 59 +++++++++++++++++++ contrib/pg_plan_advice/pgpa_planner.c | 21 ++++++- contrib/pg_plan_advice/sql/join_order.sql | 16 +++++ 3 files changed, 95 insertions(+), 1 deletion(-) diff --git a/contrib/pg_plan_advice/expected/join_order.out b/contrib/pg_plan_advice/expected/join_order.out index 7258c4f8964..76df9128703 100644 --- a/contrib/pg_plan_advice/expected/join_order.out +++ b/contrib/pg_plan_advice/expected/join_order.out @@ -249,6 +249,65 @@ SELECT * FROM jo_fact f NO_GATHER(f d1 d2) (18 rows) +COMMIT; +-- Test cases for sublists spanning all targets. +BEGIN; +SET LOCAL pg_plan_advice.advice = 'join_order((f d1 d2))'; +EXPLAIN (COSTS OFF, PLAN_ADVICE) +SELECT * FROM jo_fact f + LEFT JOIN jo_dim1 d1 ON f.dim1_id = d1.id + LEFT JOIN jo_dim2 d2 ON f.dim2_id = d2.id + WHERE val1 = 1 AND val2 = 1; + QUERY PLAN +------------------------------------------ + Hash Join + Hash Cond: (f.dim2_id = d2.id) + -> Hash Join + Hash Cond: (f.dim1_id = d1.id) + -> Seq Scan on jo_fact f + -> Hash + -> Seq Scan on jo_dim1 d1 + Filter: (val1 = 1) + -> Hash + -> Seq Scan on jo_dim2 d2 + Filter: (val2 = 1) + Supplied Plan Advice: + JOIN_ORDER((f d1 d2)) /* matched */ + Generated Plan Advice: + JOIN_ORDER(f d1 d2) + HASH_JOIN(d1 d2) + SEQ_SCAN(f d1 d2) + NO_GATHER(f d1 d2) +(18 rows) + +SET LOCAL pg_plan_advice.advice = 'join_order({f d1 d2})'; +EXPLAIN (COSTS OFF, PLAN_ADVICE) +SELECT * FROM jo_fact f + LEFT JOIN jo_dim1 d1 ON f.dim1_id = d1.id + LEFT JOIN jo_dim2 d2 ON f.dim2_id = d2.id + WHERE val1 = 1 AND val2 = 1; + QUERY PLAN +------------------------------------------ + Hash Join + Hash Cond: (f.dim1_id = d1.id) + -> Hash Join + Hash Cond: (f.dim2_id = d2.id) + -> Seq Scan on jo_fact f + -> Hash + -> Seq Scan on jo_dim2 d2 + Filter: (val2 = 1) + -> Hash + -> Seq Scan on jo_dim1 d1 + Filter: (val1 = 1) + Supplied Plan Advice: + JOIN_ORDER({f d1 d2}) /* matched */ + Generated Plan Advice: + JOIN_ORDER(f d2 d1) + HASH_JOIN(d2 d1) + SEQ_SCAN(f d2 d1) + NO_GATHER(f d1 d2) +(18 rows) + COMMIT; -- Test cases for single-element groupings. The extra grouping levels should -- be ignored. diff --git a/contrib/pg_plan_advice/pgpa_planner.c b/contrib/pg_plan_advice/pgpa_planner.c index c57df4aa7f0..9f05fa033ba 100644 --- a/contrib/pg_plan_advice/pgpa_planner.c +++ b/contrib/pg_plan_advice/pgpa_planner.c @@ -1241,14 +1241,33 @@ pgpa_join_order_permits_join(int outer_count, int inner_count, { if (child_target->ttype == PGPA_TARGET_ORDERED_LIST) { + /* + * JOIN_ORDER((...)) is treated as syntactic sugar; the + * extra level of parentheses does not change the + * interpretation. Hence, don't set sublist = true in that + * case. + */ + if (list_length(target->children) != 1) + sublist = true; target = child_target; - sublist = true; loop = true; break; } else { Assert(child_target->ttype == PGPA_TARGET_UNORDERED_LIST); + + /* + * If an unordered sublist is the entire advice target, + * for example because the user writes JOIN_ORDER({...}) + * or JOIN_ORDER(({...})), and if the join we're + * considering contains exactly the mentioned relations, + * we need to mark this advice as fully matched, because + * that won't happen anywhere else. + */ + if (itm == PGPA_ITM_EQUAL && !sublist && + list_length(target->children) == 1) + entry->flags |= PGPA_FB_MATCH_FULL; return PGPA_JO_INDIFFERENT; } } diff --git a/contrib/pg_plan_advice/sql/join_order.sql b/contrib/pg_plan_advice/sql/join_order.sql index dbc472cd3d0..48405c32ddf 100644 --- a/contrib/pg_plan_advice/sql/join_order.sql +++ b/contrib/pg_plan_advice/sql/join_order.sql @@ -82,6 +82,22 @@ SELECT * FROM jo_fact f WHERE val1 = 1 AND val2 = 1; COMMIT; +-- Test cases for sublists spanning all targets. +BEGIN; +SET LOCAL pg_plan_advice.advice = 'join_order((f d1 d2))'; +EXPLAIN (COSTS OFF, PLAN_ADVICE) +SELECT * FROM jo_fact f + LEFT JOIN jo_dim1 d1 ON f.dim1_id = d1.id + LEFT JOIN jo_dim2 d2 ON f.dim2_id = d2.id + WHERE val1 = 1 AND val2 = 1; +SET LOCAL pg_plan_advice.advice = 'join_order({f d1 d2})'; +EXPLAIN (COSTS OFF, PLAN_ADVICE) +SELECT * FROM jo_fact f + LEFT JOIN jo_dim1 d1 ON f.dim1_id = d1.id + LEFT JOIN jo_dim2 d2 ON f.dim2_id = d2.id + WHERE val1 = 1 AND val2 = 1; +COMMIT; + -- Test cases for single-element groupings. The extra grouping levels should -- be ignored. BEGIN; -- 2.53.0