Hi Robert, On Thu, Mar 12, 2026 at 10:15 AM Robert Haas <[email protected]> wrote: > I've now committed the main patch. I think that I'm not likely to get > too much more feedback on that in this release cycle, and I judge that > more people will be sad if it doesn't get shipped this cycle than if > it does. That might turn out to be catastrophically wrong, but if many > problem reports quickly begin to emerge, it can always be reverted.
I think its good to get this in, and not do it in the last minute, but just to be clear on my part, I've reviewed earlier versions, but I have not reviewed the latest code to the extent I would have liked before it was committed, due to competing priorities on my end. That said, its a good forcing function, so let me do my part to help shake out any bugs that remain. >From a code review perspective, I found some minor issues: - Identifiers that are named like advice types cause an error (so I can't name my table "hash_join") - pgpa_destroy_join_unroller does not free the inner_unrollers values (which I think goes against the intent of cleaning up the allocations right away) - pgpa_parser uses pgpa_yyerror, but that function doesn't use elog(ERROR) like the main parser does - I think it'd be more consistent to use YYABORT explicitly, e.g. how we do it in cubeparse - pgpa_scanner accepts integers with underscores, but incorrectly uses a simple strtoint on them (which would fail), instead of pg_strtoint32 / pg_strtoint32_safe - pgpa_walk_recursively uses "0" for the boolean value being passed to a recursive call in one case (should be "false") Attached nocfbot-0001 that addresses these. >From a usability perspective, I do wonder about two things when it comes to users specifying advice directly (which will happen in practice, e.g. when debugging plan problems): 1) The lack of being able to target scan advice (e.g. SEQ_SCAN) to a partition parent is frustrating - I believe we discussed partitioning earlier in the thread in terms of gathering+applying it, but I do wonder if we shouldn't at least allow users to specify a partitioned table/partitioned index, instead of only the children. See attached nocfbot-0002 for an idea what would be enough, I think. 2) I find the join strategy advice hard to use - pg_hint_plan has hints (e.g. HashJoin) that allow saying "use a hash join when joining these two rels in any order", vs pg_plan_advice requires setting a specific outer rel, which only makes sense when you want to fully specify every aspect of the plan. I suspect users who directly write advice will struggle with specifying join strategy advice as it is right now. We could consider having a different syntax for saying "I want a hash join when these two rels are joined, but I don't care about the order", e.g. "USE_HASH_JOIN(a b)". If you think that's worthwhile I'd be happy to take a stab at a patch. > I'm still hoping to get some more feedback on the remaining patches, > which are much smaller and conceptually simpler. While there is no > time to redesign them at this point in the release cycle, there is > still the opportunity to fix bugs, or decide that they're too > half-baked to ship. So here is v20 with just those patches. Of course, > post-commit review of the main patch is also very welcome. For v20-0001, from a quick conceptual review: I find the two separate GUC mechanisms for local backend vs shared memory a bit confusing as a user (which one should I be using?). Enabling the shared memory mechanism on a system-wide basis seems like it'd likely have too high overhead anyway for production systems? (specifically worried about the advice capturing for each plan, though I haven't benchmarked it) I wonder if we shouldn't keep this simpler for now, and e.g. only do the backend local version to start - we could iterate a bit on system-wide collection out-of-core, e.g. I'm considering teaching pg_stat_plans to optionally collect plan advice the first time it sees a plan ID (plan advice is roughly a superset of what we consider a plan ID there), and then we could come back to this for PG20. For v20-0002: I think we need a form of this in tree, because otherwise pg_plan_advice breaks as planner logic changes. This of course puts a burden on other hackers making changes, but I think that's what we're signing up for with having pg_plan_advice in contrib - and maybe we're overestimating how often that'll actually be a problem. To help assess impact, I did a quick test run and looked at three not-yet-committed patches in the commitfest that affect planner logic ([0], [1] and [2]), to see if they'd require pg_plan_advice changes (master with v20-0002 applied). Maybe I picked the wrong patches, but at least with those no pg_plan_advice changes were needed with the test_plan_advice test enabled. On the code itself: - Is there a reason you're setting "pg_plan_advice.always_store_advice_details" to true, instead of using pg_plan_advice_request_advice_generation? - I wonder if we could somehow detect advice not matching? Right now that'd be silently ignored, i.e. you'd only get a test failure when we generate the wrong advice that causes a plan change in the regression tests. For v20-0003, initial thoughts: I think getting at least a basic version of this in would be good, as a server-wide way to set advice for queries can help people get out of a problem when Postgres behaves badly - and we know from pg_hint_plan (which has a hint table) that this can be useful even without doing any kind of parameter sniffing/etc to be smart about different parameters for the same query. The name "stash" feels a bit confusing as an end-user facing term. Maybe something like "pg_apply_advice", or "pg_query_advice" would be better? (though I kind of wish we could tie it more closely to "plan advice", but e.g. "pg_plan_advice_apply" feels too lengthy) --- Thanks, Lukas [0]: https://commitfest.postgresql.org/patch/5487/ ("Pull-up subquery if INNER JOIN-ON contains refs to upper-query") [1]: https://commitfest.postgresql.org/patch/5738/ ("Improve hash join's handling of tuples with null join keys") [2]: https://commitfest.postgresql.org/patch/6315/ ("Enable partitionwise join for partition keys wrapped by RelabelType") -- Lukas Fittl
From a17dd9910717681cda9c6313452055ca26d802c7 Mon Sep 17 00:00:00 2001 From: Lukas Fittl <[email protected]> Date: Fri, 13 Mar 2026 00:12:46 -0700 Subject: [PATCH 2/2] pg_plan_advice: Allow targeting scans by partition parent When pg_plan_advice generates advice for a given plan, it will currently always spell out each individual partitioned table. This limitation is complex to address, but we can at least permit users to specify advice to the parent table if they want. This adjusts SEQ_SCAN and other per-scan advice to affect all children of the partition instead of the append rel itself. For INDEX_SCAN, etc. this allows specifying the name of indexes declared on the partitioned parent table, but doesn't permit indexes that only exist on the child tables. --- .../expected/partition_scan.out | 89 ++++++++++ contrib/pg_plan_advice/meson.build | 1 + contrib/pg_plan_advice/pgpa_planner.c | 105 +++++++++-- contrib/pg_plan_advice/pgpa_walker.c | 164 +++++++++++++++++- contrib/pg_plan_advice/sql/partition_scan.sql | 40 +++++ 5 files changed, 379 insertions(+), 20 deletions(-) create mode 100644 contrib/pg_plan_advice/expected/partition_scan.out create mode 100644 contrib/pg_plan_advice/sql/partition_scan.sql diff --git a/contrib/pg_plan_advice/expected/partition_scan.out b/contrib/pg_plan_advice/expected/partition_scan.out new file mode 100644 index 00000000000..c47e02c60b7 --- /dev/null +++ b/contrib/pg_plan_advice/expected/partition_scan.out @@ -0,0 +1,89 @@ +LOAD 'pg_plan_advice'; +SET max_parallel_workers_per_gather = 0; +SET seq_page_cost = 0.1; +SET random_page_cost = 0.1; +SET cpu_tuple_cost = 0; +SET cpu_index_tuple_cost = 0; +CREATE TABLE pt (a int primary key, b text) + PARTITION BY RANGE (a); +CREATE TABLE pt_1 PARTITION OF pt FOR VALUES FROM (1) TO (50001) + WITH (autovacuum_enabled = false); +CREATE TABLE pt_2 PARTITION OF pt FOR VALUES FROM (50001) TO (100001) + WITH (autovacuum_enabled = false); +INSERT INTO pt SELECT g, 'some text ' || g FROM generate_series(1, 100000) g; +VACUUM ANALYZE pt; +-- By default this does a sequential scan on each partition. +EXPLAIN (COSTS OFF) SELECT * FROM pt; + QUERY PLAN +------------------------ + Append + -> Seq Scan on pt_1 + -> Seq Scan on pt_2 +(3 rows) + +-- By default a lookup by primary key uses an index scan. +EXPLAIN (COSTS OFF) SELECT * FROM pt WHERE a = 1; + QUERY PLAN +--------------------------------------- + Index Scan using pt_1_pkey on pt_1 pt + Index Cond: (a = 1) +(2 rows) + +-- SEQ_SCAN on the parent should force seq scan on all children, even when +-- an index scan would normally be used. +BEGIN; +SET LOCAL pg_plan_advice.advice = 'SEQ_SCAN(pt)'; +EXPLAIN (COSTS OFF, PLAN_ADVICE) SELECT * FROM pt WHERE a = 1; + QUERY PLAN +------------------------------ + Seq Scan on pt_1 pt + Filter: (a = 1) + Supplied Plan Advice: + SEQ_SCAN(pt) /* matched */ + Generated Plan Advice: + SEQ_SCAN(pt/public.pt_1) + PARTITIONWISE(pt) + NO_GATHER(pt/public.pt_1) +(8 rows) + +COMMIT; +-- INDEX_SCAN on the parent should force index scan on all children. +BEGIN; +SET LOCAL pg_plan_advice.advice = 'INDEX_SCAN(pt pt_pkey)'; +EXPLAIN (COSTS OFF, PLAN_ADVICE) SELECT * FROM pt WHERE a > 0; + QUERY PLAN +------------------------------------------------------------------------------- + Append + -> Index Scan using pt_1_pkey on pt_1 + Index Cond: (a > 0) + -> Index Scan using pt_2_pkey on pt_2 + Index Cond: (a > 0) + Supplied Plan Advice: + INDEX_SCAN(pt pt_pkey) /* matched */ + Generated Plan Advice: + INDEX_SCAN(pt/public.pt_1 public.pt_1_pkey pt/public.pt_2 public.pt_2_pkey) + PARTITIONWISE(pt) + NO_GATHER(pt/public.pt_1 pt/public.pt_2) +(11 rows) + +COMMIT; +-- TID_SCAN on the parent should force TID scan on all children. +BEGIN; +SET LOCAL pg_plan_advice.advice = 'TID_SCAN(pt)'; +EXPLAIN (COSTS OFF, PLAN_ADVICE) SELECT * FROM pt WHERE ctid = '(0,1)'; + QUERY PLAN +-------------------------------------------- + Append + -> Tid Scan on pt_1 + TID Cond: (ctid = '(0,1)'::tid) + -> Tid Scan on pt_2 + TID Cond: (ctid = '(0,1)'::tid) + Supplied Plan Advice: + TID_SCAN(pt) /* matched */ + Generated Plan Advice: + PARTITIONWISE(pt) + TID_SCAN(pt/public.pt_1 pt/public.pt_2) + NO_GATHER(pt/public.pt_1 pt/public.pt_2) +(11 rows) + +COMMIT; diff --git a/contrib/pg_plan_advice/meson.build b/contrib/pg_plan_advice/meson.build index cf948ffaa13..7b753c0acdb 100644 --- a/contrib/pg_plan_advice/meson.build +++ b/contrib/pg_plan_advice/meson.build @@ -56,6 +56,7 @@ tests += { 'gather', 'join_order', 'join_strategy', + 'partition_scan', 'partitionwise', 'prepared', 'scan', diff --git a/contrib/pg_plan_advice/pgpa_planner.c b/contrib/pg_plan_advice/pgpa_planner.c index 5508b8af707..f64126d6f36 100644 --- a/contrib/pg_plan_advice/pgpa_planner.c +++ b/contrib/pg_plan_advice/pgpa_planner.c @@ -32,6 +32,8 @@ #include "optimizer/plancat.h" #include "optimizer/planner.h" #include "parser/parsetree.h" +#include "catalog/partition.h" +#include "catalog/pg_class.h" #include "utils/lsyscache.h" #ifdef USE_ASSERT_CHECKING @@ -471,16 +473,48 @@ pgpa_build_simple_rel(PlannerInfo *root, RelOptInfo *rel, RangeTblEntry *rte) if (pps != NULL && pps->trove != NULL) { pgpa_identifier rid; - pgpa_trove_result tresult_scan; + pgpa_trove_result tresult_scan = {0}; pgpa_trove_result tresult_rel; /* Search for scan advice and general rel advice. */ pgpa_compute_identifier_by_rti(root, rel->relid, &rid); - pgpa_trove_lookup(pps->trove, PGPA_TROVE_LOOKUP_SCAN, 1, &rid, - &tresult_scan); + + /* + * Skip scan advice lookup for partitioned table parents. Scan + * methods are only meaningful for the leaf partitions, not the parent + * rel whose plan is an Append over child scans. Applying scan advice + * to the parent would incorrectly disable PGS_APPEND. Parent-level + * scan advice cascades to children below instead. + */ + if (rte->relkind != RELKIND_PARTITIONED_TABLE) + pgpa_trove_lookup(pps->trove, PGPA_TROVE_LOOKUP_SCAN, 1, &rid, + &tresult_scan); pgpa_trove_lookup(pps->trove, PGPA_TROVE_LOOKUP_REL, 1, &rid, &tresult_rel); + /* + * If this is a child partition, also look up scan advice targeting + * the parent table. Scan advice like SEQ_SCAN(parent) should cascade + * to all child partitions, not just the parent rel itself. We only do + * this for scan advice, not rel advice, because rel advice like + * PARTITIONWISE applies to the parent conceptually and should not be + * re-applied to individual children. + */ + if (rid.partrel != NULL) + { + pgpa_identifier parent_rid; + pgpa_trove_result tresult_parent_scan; + + parent_rid = rid; + parent_rid.partnsp = NULL; + parent_rid.partrel = NULL; + pgpa_trove_lookup(pps->trove, PGPA_TROVE_LOOKUP_SCAN, 1, + &parent_rid, &tresult_parent_scan); + tresult_scan.indexes = bms_union(tresult_scan.indexes, + tresult_parent_scan.indexes); + tresult_scan.entries = tresult_parent_scan.entries; + } + /* If relevant entries were found, apply them. */ if (tresult_scan.indexes != NULL || tresult_rel.indexes != NULL) { @@ -1670,6 +1704,54 @@ pgpa_semijoin_permits_join(int outer_count, int inner_count, /* * Apply scan advice to a RelOptInfo. */ +/* + * Find an index in the relation's index list matching the given target. + * + * First tries a direct name match. If that fails and the index is a child + * partition index, also checks whether the target names a parent index from + * which this child index inherits. This allows advice like + * INDEX_SCAN(parent parent_idx) to cascade to child partitions whose indexes + * have different names. + */ +static IndexOptInfo * +pgpa_find_matching_index(RelOptInfo *rel, pgpa_index_target *itarget) +{ + foreach_node(IndexOptInfo, index, rel->indexlist) + { + char *relname = get_rel_name(index->indexoid); + Oid nspoid = get_rel_namespace(index->indexoid); + char *relnamespace = get_namespace_name_or_temp(nspoid); + + /* Direct name match. */ + if (strcmp(itarget->indname, relname) == 0 && + (itarget->indnamespace == NULL || + strcmp(itarget->indnamespace, relnamespace) == 0)) + return index; + + /* Check whether the target names a parent index. */ + if (get_rel_relispartition(index->indexoid)) + { + Oid parent_idx; + + parent_idx = get_partition_parent(index->indexoid, true); + if (OidIsValid(parent_idx)) + { + char *parent_name = get_rel_name(parent_idx); + Oid parent_nspoid = get_rel_namespace(parent_idx); + char *parent_nsp = + get_namespace_name_or_temp(parent_nspoid); + + if (strcmp(itarget->indname, parent_name) == 0 && + (itarget->indnamespace == NULL || + strcmp(itarget->indnamespace, parent_nsp) == 0)) + return index; + } + } + } + + return NULL; +} + static void pgpa_planner_apply_scan_advice(RelOptInfo *rel, pgpa_trove_entry *scan_entries, @@ -1826,22 +1908,7 @@ pgpa_planner_apply_scan_advice(RelOptInfo *rel, scan_entry->tag == PGPA_TAG_INDEX_ONLY_SCAN)) { pgpa_index_target *itarget = scan_entry->target->itarget; - IndexOptInfo *matched_index = NULL; - - foreach_node(IndexOptInfo, index, rel->indexlist) - { - char *relname = get_rel_name(index->indexoid); - Oid nspoid = get_rel_namespace(index->indexoid); - char *relnamespace = get_namespace_name_or_temp(nspoid); - - if (strcmp(itarget->indname, relname) == 0 && - (itarget->indnamespace == NULL || - strcmp(itarget->indnamespace, relnamespace) == 0)) - { - matched_index = index; - break; - } - } + IndexOptInfo *matched_index = pgpa_find_matching_index(rel, itarget); if (matched_index == NULL) { diff --git a/contrib/pg_plan_advice/pgpa_walker.c b/contrib/pg_plan_advice/pgpa_walker.c index c1203f123a5..4e53c20cf03 100644 --- a/contrib/pg_plan_advice/pgpa_walker.c +++ b/contrib/pg_plan_advice/pgpa_walker.c @@ -15,6 +15,8 @@ #include "pgpa_scan.h" #include "pgpa_walker.h" +#include "catalog/partition.h" +#include "nodes/pathnodes.h" #include "nodes/plannodes.h" #include "parser/parsetree.h" #include "utils/lsyscache.h" @@ -45,6 +47,14 @@ static bool pgpa_walker_join_order_matches_member(pgpa_join_member *member, Index rtable_length, pgpa_identifier *rt_identifiers, pgpa_advice_target *target); +static pgpa_scan_strategy pgpa_scan_tag_to_strategy(pgpa_advice_tag_type tag); +static Bitmapset *pgpa_walker_find_child_rtis(Index parent_rti, + Index rtable_length, + pgpa_identifier *rt_identifiers); +static bool pgpa_walker_check_children_scan(pgpa_plan_walker_context *walker, + pgpa_scan_strategy strategy, + Bitmapset *child_rtis, + pgpa_index_target *itarget); static pgpa_scan *pgpa_walker_find_scan(pgpa_plan_walker_context *walker, pgpa_scan_strategy strategy, Bitmapset *relids); @@ -698,6 +708,30 @@ pgpa_walker_would_advise(pgpa_plan_walker_context *walker, if (rti == 0) return false; relids = bms_make_singleton(rti); + + /* + * For scan advice targeting a parent table (no partition specified), + * check whether child partitions exist. If so, validate the advice + * against the children, since scans are recorded against child + * partitions rather than the parent. + */ + if (target->rid.partrel == NULL) + { + Bitmapset *child_rtis; + + child_rtis = pgpa_walker_find_child_rtis(rti, rtable_length, + rt_identifiers); + if (child_rtis != NULL) + { + pgpa_scan_strategy strategy; + + strategy = pgpa_scan_tag_to_strategy(tag); + if (strategy != PGPA_SCAN_ORDINARY) + return pgpa_walker_check_children_scan(walker, strategy, + child_rtis, + target->itarget); + } + } } else { @@ -843,7 +877,35 @@ pgpa_walker_index_target_matches_plan(pgpa_index_target *itarget, Plan *plan) } /* Check whether relation name matches. */ - return (strcmp(itarget->indname, get_rel_name(indexoid)) == 0); + if (strcmp(itarget->indname, get_rel_name(indexoid)) == 0) + return true; + + /* + * For child partition indexes, also check whether the target names the + * parent index from which this child index inherits. + */ + if (get_rel_relispartition(indexoid)) + { + Oid parent_idx = get_partition_parent(indexoid, true); + + if (OidIsValid(parent_idx)) + { + if (itarget->indnamespace != NULL) + { + Oid parent_nspoid = get_rel_namespace(parent_idx); + char *parent_nsp = + get_namespace_name_or_temp(parent_nspoid); + + if (strcmp(itarget->indnamespace, parent_nsp) != 0) + return false; + } + + if (strcmp(itarget->indname, get_rel_name(parent_idx)) == 0) + return true; + } + } + + return false; } /* @@ -978,6 +1040,106 @@ pgpa_walker_find_scan(pgpa_plan_walker_context *walker, return NULL; } +/* + * Map a scan-related advice tag to a scan strategy. + * + * Returns PGPA_SCAN_ORDINARY if the tag is not a simple scan tag (e.g., + * DISABLE_INDEX, INDEX_SCAN with index target, PARTITIONWISE, or non-scan + * tags), in which case the caller should fall through to the normal logic. + */ +static pgpa_scan_strategy +pgpa_scan_tag_to_strategy(pgpa_advice_tag_type tag) +{ + switch (tag) + { + case PGPA_TAG_BITMAP_HEAP_SCAN: + return PGPA_SCAN_BITMAP_HEAP; + case PGPA_TAG_INDEX_SCAN: + return PGPA_SCAN_INDEX; + case PGPA_TAG_INDEX_ONLY_SCAN: + return PGPA_SCAN_INDEX_ONLY; + case PGPA_TAG_SEQ_SCAN: + return PGPA_SCAN_SEQ; + case PGPA_TAG_TID_SCAN: + return PGPA_SCAN_TID; + default: + return PGPA_SCAN_ORDINARY; + } +} + +/* + * Find child partition RTIs for a given parent RTI. + * + * If the given RTI corresponds to a parent table identifier (partrel == NULL), + * returns a Bitmapset of all child RTIs that share the same alias_name, + * occurrence, and plan_name. Returns NULL if no children exist. + */ +static Bitmapset * +pgpa_walker_find_child_rtis(Index parent_rti, Index rtable_length, + pgpa_identifier *rt_identifiers) +{ + pgpa_identifier *parent_rid = &rt_identifiers[parent_rti - 1]; + Bitmapset *child_rtis = NULL; + + /* Only applicable when the parent has no partition name. */ + if (parent_rid->alias_name == NULL || parent_rid->partrel != NULL) + return NULL; + + for (Index rti = 1; rti <= rtable_length; ++rti) + { + pgpa_identifier *rid = &rt_identifiers[rti - 1]; + + if (rid->alias_name == NULL || rid->partrel == NULL) + continue; + if (strcmp(rid->alias_name, parent_rid->alias_name) != 0) + continue; + if (rid->occurrence != parent_rid->occurrence) + continue; + if (!strings_equal_or_both_null(rid->plan_name, parent_rid->plan_name)) + continue; + + child_rtis = bms_add_member(child_rtis, rti); + } + + return child_rtis; +} + +/* + * Check scan advice against child partitions of a parent table. + * + * Returns true if every scanned child partition uses the specified scan + * strategy. + * + * Children that were pruned away (no scan recorded) are ignored. + */ +static bool +pgpa_walker_check_children_scan(pgpa_plan_walker_context *walker, + pgpa_scan_strategy strategy, + Bitmapset *child_rtis, + pgpa_index_target *itarget) +{ + bool found_any = false; + int rti = -1; + + while ((rti = bms_next_member(child_rtis, rti)) >= 0) + { + Bitmapset *child_relids = bms_make_singleton(rti); + pgpa_scan *scan = pgpa_walker_find_scan(walker, strategy, + child_relids); + + if (scan != NULL) + { + /* For INDEX_SCAN/INDEX_ONLY_SCAN, also verify the index. */ + if (itarget != NULL && + !pgpa_walker_index_target_matches_plan(itarget, scan->plan)) + continue; + found_any = true; + } + } + + return found_any; +} + /* * Does this walker say that the given query feature applies to the given * relid set? diff --git a/contrib/pg_plan_advice/sql/partition_scan.sql b/contrib/pg_plan_advice/sql/partition_scan.sql new file mode 100644 index 00000000000..ad462b61695 --- /dev/null +++ b/contrib/pg_plan_advice/sql/partition_scan.sql @@ -0,0 +1,40 @@ +LOAD 'pg_plan_advice'; +SET max_parallel_workers_per_gather = 0; +SET seq_page_cost = 0.1; +SET random_page_cost = 0.1; +SET cpu_tuple_cost = 0; +SET cpu_index_tuple_cost = 0; + +CREATE TABLE pt (a int primary key, b text) + PARTITION BY RANGE (a); +CREATE TABLE pt_1 PARTITION OF pt FOR VALUES FROM (1) TO (50001) + WITH (autovacuum_enabled = false); +CREATE TABLE pt_2 PARTITION OF pt FOR VALUES FROM (50001) TO (100001) + WITH (autovacuum_enabled = false); +INSERT INTO pt SELECT g, 'some text ' || g FROM generate_series(1, 100000) g; +VACUUM ANALYZE pt; + +-- By default this does a sequential scan on each partition. +EXPLAIN (COSTS OFF) SELECT * FROM pt; + +-- By default a lookup by primary key uses an index scan. +EXPLAIN (COSTS OFF) SELECT * FROM pt WHERE a = 1; + +-- SEQ_SCAN on the parent should force seq scan on all children, even when +-- an index scan would normally be used. +BEGIN; +SET LOCAL pg_plan_advice.advice = 'SEQ_SCAN(pt)'; +EXPLAIN (COSTS OFF, PLAN_ADVICE) SELECT * FROM pt WHERE a = 1; +COMMIT; + +-- INDEX_SCAN on the parent should force index scan on all children. +BEGIN; +SET LOCAL pg_plan_advice.advice = 'INDEX_SCAN(pt pt_pkey)'; +EXPLAIN (COSTS OFF, PLAN_ADVICE) SELECT * FROM pt WHERE a > 0; +COMMIT; + +-- TID_SCAN on the parent should force TID scan on all children. +BEGIN; +SET LOCAL pg_plan_advice.advice = 'TID_SCAN(pt)'; +EXPLAIN (COSTS OFF, PLAN_ADVICE) SELECT * FROM pt WHERE ctid = '(0,1)'; +COMMIT; -- 2.47.1
From 25d299dfe5761e2354af12c856e7d7cb1dc3333d Mon Sep 17 00:00:00 2001 From: Lukas Fittl <[email protected]> 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
