On Thu, Oct 30, 2025 at 3:00 PM Robert Haas <[email protected]> wrote:
[..over 400kB of attachments, yay]
Thank You for working on this!
My gcc-13 was nitpicking a little bit (see
compilation_warnings_v1.txt), so attached is just a tiny diff to fix
some of those issues. After that, clang-20 run was clean too.
> First, any form of user control over the planner tends to be a lightning rod
> for criticism around here.
I do not know where this is coming from, but everybody I've talked to
was saying this is needed to handle real enterprise databases and
applications. I just really love it, how one could precisely adjust
the plan with this even with the presence of heavy aliasing:
postgres=# explain (plan_advice, costs off) SELECT * FROM (select *
from t1 a join t2 b using (id)) a, t2 b, t3 c WHERE a.id = b.id and
b.id = c.id;
QUERY PLAN
-----------------------------------------------------
Merge Join
Merge Cond: (a.id = c.id)
-> Merge Join
Merge Cond: (a.id = b.id)
-> Index Scan using t1_pkey on t1 a
-> Index Scan using t2_pkey on t2 b
-> Sort
Sort Key: c.id
-> Seq Scan on t3 c
Supplied Plan Advice:
SEQ_SCAN(ble5) /* not matched */
Generated Plan Advice:
JOIN_ORDER(a#2 b#2 c)
MERGE_JOIN_PLAIN(b#2 c)
SEQ_SCAN(c)
INDEX_SCAN(a#2 public.t1_pkey )
NO_GATHER(c a#2 b#2)
(17 rows)
postgres=# set pg_plan_advice.advice = 'SEQ_SCAN(b#2)';
SET
postgres=# explain (plan_advice, costs off) SELECT * FROM (select *
from t1 a join t2 b using (id)) a, t2 b, t3 c WHERE a.id = b.id and
b.id = c.id;
QUERY PLAN
----------------------------------------------------
Hash Join
Hash Cond: (b.id = a.id)
-> Seq Scan on t2 b
-> Hash
-> Merge Join
Merge Cond: (a.id = c.id)
-> Index Scan using t1_pkey on t1 a
-> Sort
Sort Key: c.id
-> Seq Scan on t3 c
Supplied Plan Advice:
SEQ_SCAN(b#2) /* matched */
Generated Plan Advice:
JOIN_ORDER(b#2 (a#2 c))
MERGE_JOIN_PLAIN(c)
HASH_JOIN(c)
SEQ_SCAN(b#2 c)
INDEX_SCAN(a#2 public.t1_pkey)
NO_GATHER(c a#2 b#2)
To attract a little attention to the $thread, the only bigger design
(usability) question that keeps ringing in my head is how we are going
to bind it to specific queries without even issuing any SETs(or ALTER
USER) in the far future in the grand scheme of things. The discussed
query id (hash), full query text comparison, maybe even strstr(query ,
"partial hit") or regex all seem to be kind too limited in terms of
what crazy ORMs can come up with (each query will be potentially
slightly different, but if optimizer reference points are stable that
should nail it good enough, but just enabling it for the very specific
set of queries and not the others [with same aliases] is some major
challenge).
Due to this, at some point I was even thinking about some hashes for
every plan node (including hashes of subplans), e.g.:
Merge Join // hash(MERGE_JOIN_PLAIN(b#2) + ';' somehashval1 + ';'+
somehahsval2 ) => somehashval3
Merge Cond: (a.id = c.id)
-> Merge Join
Merge Cond: (a.id = b.id)
-> Index Scan using t1_pkey on t1 a // hash(INDEX_SCAN(a#2
public.t1_pkey)) => somehashval1
-> Index Scan using t2_pkey on t2 b // hash(INDEX_SCAN(b#2
public.t2_pkey)) => somehashval2
and then having a way to use `somehashval3` (let's say it's SHA1) as a
way to activate the necessary advice. Something like having a way to
express it using plan_advice.on_subplanhashes_plan_advice =
'somehashval3: SEQ_SCAN(b#2)'. This would have the benefit of being
able to override multiple similiar SQL queries in one go rather than
collecting all possible query_ids, but probably it's stupid,
heavyweight, but that would be my dream ;)
-J.
From 06038e237420b7054912118076e8b981def4f545 Mon Sep 17 00:00:00 2001
From: Jakub Wartak <[email protected]>
Date: Fri, 31 Oct 2025 09:35:59 +0100
Subject: [PATCH] fixup: supress some gcc warnings
---
contrib/pg_plan_advice/pgpa_ast.c | 3 ++-
contrib/pg_plan_advice/pgpa_output.c | 12 ++++++++----
contrib/pg_plan_advice/pgpa_walker.c | 4 +++-
3 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/contrib/pg_plan_advice/pgpa_ast.c
b/contrib/pg_plan_advice/pgpa_ast.c
index ed18950af18..be598874c48 100644
--- a/contrib/pg_plan_advice/pgpa_ast.c
+++ b/contrib/pg_plan_advice/pgpa_ast.c
@@ -138,7 +138,8 @@ pgpa_cstring_advice_tag(pgpa_advice_tag_type advice_tag)
return "TID_SCAN";
}
- Assert(false);
+ elog(ERROR, "unrecognized advice type: %d", advice_tag);
+ pg_unreachable();
}
/*
diff --git a/contrib/pg_plan_advice/pgpa_output.c
b/contrib/pg_plan_advice/pgpa_output.c
index 2175278b580..5aae5071990 100644
--- a/contrib/pg_plan_advice/pgpa_output.c
+++ b/contrib/pg_plan_advice/pgpa_output.c
@@ -10,6 +10,7 @@
*-------------------------------------------------------------------------
*/
+#include "c.h"
#include "postgres.h"
#include "pgpa_output.h"
@@ -507,7 +508,8 @@ pgpa_cstring_join_strategy(pgpa_join_strategy strategy)
return "HASH_JOIN";
}
- Assert(false);
+ elog(ERROR, "unrecognized join strategy: %d", strategy);
+ pg_unreachable();
}
/*
@@ -536,11 +538,12 @@ pgpa_cstring_scan_strategy(pgpa_scan_strategy strategy)
return "TID_SCAN";
}
- Assert(false);
+ elog(ERROR, "unrecognized scan strategy: %d", strategy);
+ pg_unreachable();
}
/*
- * Get a C string that corresponds to the specified scan strategy.
+ * Get a C string that corresponds to the specified query feature type.
*/
static char *
pgpa_cstring_query_feature_type(pgpa_qf_type type)
@@ -557,7 +560,8 @@ pgpa_cstring_query_feature_type(pgpa_qf_type type)
return "SEMIJOIN_UNIQUE";
}
- Assert(false);
+ elog(ERROR, "unrecognized query feature type: %d", type);
+ pg_unreachable();
}
/*
diff --git a/contrib/pg_plan_advice/pgpa_walker.c
b/contrib/pg_plan_advice/pgpa_walker.c
index d22ac11bf91..44adeb4511b 100644
--- a/contrib/pg_plan_advice/pgpa_walker.c
+++ b/contrib/pg_plan_advice/pgpa_walker.c
@@ -359,7 +359,7 @@ pgpa_walk_recursively(pgpa_plan_walker_context *walker,
Plan *plan,
{
int num_aqf =
list_length(active_query_features);
- (void) list_truncate(active_query_features, num_aqf - 1);
+ active_query_features = list_truncate(active_query_features,
num_aqf - 1);
}
}
@@ -768,6 +768,7 @@ pgpa_walker_join_order_matches_member(pgpa_join_member
*member,
relids = bms_add_member(relids, rti);
}
}
+ break;
case PGPA_TARGET_IDENTIFIER:
{
@@ -777,6 +778,7 @@ pgpa_walker_join_order_matches_member(pgpa_join_member
*member,
&target->rid);
relids = bms_make_singleton(rti);
}
+ break;
}
return bms_equal(member->scan->relids, relids);
--
2.43.0
From 4fdb5d6b047fc7fd188be23c64ae7f352b014f21 Mon Sep 17 00:00:00 2001
From: Jakub Wartak <[email protected]>
Date: Fri, 31 Oct 2025 09:34:58 +0100
Subject: [PATCH 1/2] fixup: not sure
---
contrib/pg_plan_advice/pgpa_planner.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/contrib/pg_plan_advice/pgpa_planner.c
b/contrib/pg_plan_advice/pgpa_planner.c
index c4859c23020..5b7d2cbd9f4 100644
--- a/contrib/pg_plan_advice/pgpa_planner.c
+++ b/contrib/pg_plan_advice/pgpa_planner.c
@@ -1411,7 +1411,7 @@ pgpa_planner_apply_scan_advice(RelOptInfo *rel,
* Note that setting PGS_CONSIDER_NONPARTIAL in my_gather_mask
is
* equivalent to allowing the non-use of either form of Gather
here.
*/
- if (my_entry->tag == PGPA_TAG_GATHER |
+ if (my_entry->tag == PGPA_TAG_GATHER ||
my_entry->tag == PGPA_TAG_GATHER_MERGE)
{
if (!just_one_rel)
--
2.43.0
../contrib/pg_plan_advice/pgpa_ast.c: In function ‘pgpa_cstring_advice_tag’:
../contrib/pg_plan_advice/pgpa_ast.c:142:1: warning: control reaches end of
non-void function [-Wreturn-type]
142 | }
| ^
../contrib/pg_plan_advice/pgpa_output.c: In function
‘pgpa_cstring_join_strategy’:
../contrib/pg_plan_advice/pgpa_output.c:511:1: warning: control reaches end of
non-void function [-Wreturn-type]
511 | }
| ^
../contrib/pg_plan_advice/pgpa_output.c: In function
‘pgpa_cstring_scan_strategy’:
../contrib/pg_plan_advice/pgpa_output.c:540:1: warning: control reaches end of
non-void function [-Wreturn-type]
540 | }
| ^
../contrib/pg_plan_advice/pgpa_output.c: In function
‘pgpa_cstring_query_feature_type’:
../contrib/pg_plan_advice/pgpa_output.c:561:1: warning: control reaches end of
non-void function [-Wreturn-type]
561 | }
| ^
../contrib/pg_plan_advice/pgpa_walker.c: In function ‘pgpa_walk_recursively’:
../contrib/pg_plan_advice/pgpa_walker.c:362:24: warning: ignoring return value
of ‘list_truncate’ declared with attribute ‘warn_unused_result’
[-Wunused-result]
362 | (void) list_truncate(active_query_features, num_aqf -
1);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from ../src/include/nodes/primnodes.h:23,
from ../src/include/nodes/plannodes.h:23,
from ../contrib/pg_plan_advice/pgpa_join.h:15,
from ../contrib/pg_plan_advice/pgpa_walker.c:14:
../contrib/pg_plan_advice/pgpa_walker.c: In function
‘pgpa_walker_join_order_matches_member’:
../src/include/nodes/pg_list.h:482:9: warning: this statement may fall through
[-Wimplicit-fallthrough=]
482 | for (type pointer var = 0, pointer var##__outerloop = (type
pointer) 1; \
| ^~~
../src/include/nodes/pg_list.h:469:37: note: in expansion of macro
‘foreach_internal’
469 | #define foreach_ptr(type, var, lst) foreach_internal(type, *, var, lst,
lfirst)
| ^~~~~~~~~~~~~~~~
../contrib/pg_plan_advice/pgpa_walker.c:762:33: note: in expansion of macro
‘foreach_ptr’
762 | foreach_ptr(pgpa_advice_target,
child_target, target->children)
| ^~~~~~~~~~~
../contrib/pg_plan_advice/pgpa_walker.c:772:17: note: here
772 | case PGPA_TARGET_IDENTIFIER:
| ^~~~
../contrib/pg_plan_advice/pgpa_planner.c: In function
‘pgpa_planner_apply_scan_advice’:
../contrib/pg_plan_advice/pgpa_planner.c:1414:35: warning: suggest parentheses
around comparison in operand of ‘|’ [-Wparentheses]
1414 | if (my_entry->tag == PGPA_TAG_GATHER |
| ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
