On 12.05.2025 14:05, Tom Lane wrote:
Robert Haas<robertmh...@gmail.com> writes:
But that's also assuming that you're correct here about how to descend
through a JoinExpr, which I'm not quite sure whether is true. It's
also assuming that we should solve the problem here rather than in
some other part of the code e.g. the join removal code, and I'm not
sure about that either.
The actual problem here is that remove_useless_joins hasn't run yet.
It's called inside query_planner which happens only after we do
preprocess_minmax_aggregates.
So I think this patch is a dead end. It's not possible for it to
correctly predict whether remove_useless_joins will remove the join,
short of repeating all that work which we surely don't want.
(I'm a bit surprised that it hasn't visibly broken existing test cases.)
To be honest, I was not completely sure about my decision at first and
had no idea how to do it differently, so I submitted a request for
"Advanced session feedback" to consider this patch.
It might be possible to move preprocess_minmax_aggregates to happen
after join removal, but I fear it'd require some pretty fundamental
rethinking of how it generates indexscan paths --- recursively
calling query_planner seems dubious. (But maybe that'd work?
The modified query should no longer contain aggs, so we wouldn't
recurse again.)
I considered another approach using late optimization and ran into a
problem where the planner could not find a partitioned table.
It was a long time ago to be frankly, but the problem there was that the
planner stored this information at a higher level. I can try to finish this.
I attached a diff just in case.
preprocess_minmax_aggregates is pretty much of a hack anyway.
If you read the comments, it's just full of weird stuff that it
has to duplicate from other places, or things that magically work
because the relevant stuff isn't possible in this query, etc.
Maybe it's time to think about nuking it from orbit and doing a
fresh implementation in some other place that's a better fit.
I have no immediate ideas about what that should look like, other
than it'd be better if it happened after join removal.
I didn't consider this and I'll think about it.
Thanks for the feedback!
Regards,
Alena Rybakina
Postgres Professional
diff --git a/src/backend/optimizer/plan/planagg.c
b/src/backend/optimizer/plan/planagg.c
index
64605be31781f9e841f02ea005e9220a721e45aa..c78f32e850bb2bf884a63971898de2c6a88675ea
100644
--- a/src/backend/optimizer/plan/planagg.c
+++ b/src/backend/optimizer/plan/planagg.c
@@ -125,16 +125,24 @@ preprocess_minmax_aggregates(PlannerInfo *root)
return;
jtnode = linitial(jtnode->fromlist);
}
- if (!IsA(jtnode, RangeTblRef))
+ if (IsA(jtnode, JoinExpr) && jtnode->fromlist == NIL &&
root->join_info_list == NIL && IsA(jtnode->quals, RangeTblRef))
+ rtr = (RangeTblRef *) (jtnode->quals);
+ else if (IsA(jtnode, RangeTblRef))
+ {
+ rtr = (RangeTblRef *) jtnode;
+ }
+ else
+ return;
+
+ if(root->simple_rel_array[rtr->rtindex] == NULL)
return;
- rtr = (RangeTblRef *) jtnode;
+
rte = planner_rt_fetch(rtr->rtindex, root);
+
if (rte->rtekind == RTE_RELATION)
- /* ordinary relation, ok */ ;
+ /* ordinary relation, ok */ ;
else if (rte->rtekind == RTE_SUBQUERY && rte->inh)
- /* flattened UNION ALL subquery, ok */ ;
- else
- return;
+ /* flattened UNION ALL subquery, ok */ ;
/*
* Examine all the aggregates and verify all are MIN/MAX aggregates.
Stop
@@ -345,6 +353,7 @@ build_minmax_path(PlannerInfo *root, MinMaxAggInfo *mminfo,
subroot->init_plans = NIL;
subroot->agginfos = NIL;
subroot->aggtransinfos = NIL;
+ subroot->eq_classes = NIL;
subroot->parse = parse = copyObject(root->parse);
IncrementVarSublevelsUp((Node *) parse, 1, 1);
diff --git a/src/backend/optimizer/plan/planmain.c
b/src/backend/optimizer/plan/planmain.c
index
ade23fd9d56d2a96ff62ceeeea67869aa96e368a..563e689dfc0c3f2e8aad2dea367a928e0467598a
100644
--- a/src/backend/optimizer/plan/planmain.c
+++ b/src/backend/optimizer/plan/planmain.c
@@ -78,6 +78,7 @@ query_planner(PlannerInfo *root,
root->placeholder_array_size = 0;
root->fkey_list = NIL;
root->initial_rels = NIL;
+ root->placeholdersFrozen = false;
/*
* Set up arrays for accessing base relations and AppendRelInfos.
diff --git a/src/backend/optimizer/plan/planner.c
b/src/backend/optimizer/plan/planner.c
index
8a474a50be73c2a73607ee2c775758ac95999b79..9627ea2aed196b3bcb46ff82f2a9d6535473a1d0
100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -1535,15 +1535,6 @@ grouping_planner(PlannerInfo *root, double
tuple_fraction,
parse->hasWindowFuncs = false;
}
- /*
- * Preprocess MIN/MAX aggregates, if any. Note: be careful
about
- * adding logic between here and the query_planner() call.
Anything
- * that is needed in MIN/MAX-optimizable cases will have to be
- * duplicated in planagg.c.
- */
- if (parse->hasAggs)
- preprocess_minmax_aggregates(root);
-
/*
* Figure out whether there's a hard limit on the number of
rows that
* query_planner's result subplan needs to return. Even if we
know a
@@ -1612,6 +1603,15 @@ grouping_planner(PlannerInfo *root, double
tuple_fraction,
sort_input_target_parallel_safe =
final_target_parallel_safe;
}
+ /*
+ * Preprocess MIN/MAX aggregates, if any. Note: be careful
about
+ * adding logic between here and the query_planner() call.
Anything
+ * that is needed in MIN/MAX-optimizable cases will have to be
+ * duplicated in planagg.c.
+ */
+ if (parse->hasAggs)
+ preprocess_minmax_aggregates(root);
+
/*
* If we have window functions to deal with, the output from any
* grouping step needs to be what the window functions want;
diff --git a/src/test/regress/expected/aggregates.out
b/src/test/regress/expected/aggregates.out
index
f2fb66388cc91934bf90faeaea7bc06751b14670..4cab1539cffd075f870d04b2639a33a33cbb8f2d
100644
--- a/src/test/regress/expected/aggregates.out
+++ b/src/test/regress/expected/aggregates.out
@@ -1329,6 +1329,24 @@ from int4_tbl t0;
(5 rows)
rollback;
+--Check min/max optimization for join expressions
+create unique index tenk1_unique1_idx on tenk1(unique1);
+create index tenk1_unique2_idx on tenk1(unique2);
+create unique index INT4_TBL_f1_idx on INT4_TBL(f1);
+explain (COSTS OFF)
+select min(t1.unique2) from tenk1 t1 left join INT4_TBL t2 on t1.unique1 =
t2.f1;
+ QUERY PLAN
+--------------------------------------------------------------
+ Result
+ InitPlan 1
+ -> Limit
+ -> Index Scan using tenk1_unique2_idx on tenk1 t1
+ Index Cond: (unique2 IS NOT NULL)
+(5 rows)
+
+DROP INDEX tenk1_unique1_idx;
+DROP INDEX tenk1_unique2_idx;
+DROP INDEX INT4_TBL_f1_idx;
-- check for correct detection of nested-aggregate errors
select max(min(unique1)) from tenk1;
ERROR: aggregate function calls cannot be nested
diff --git a/src/test/regress/sql/aggregates.sql
b/src/test/regress/sql/aggregates.sql
index
77168bcc7447c5d8dec11cd133c9355f82b33235..9b01b01e928af84f98484b17fd8c66134fa33a77
100644
--- a/src/test/regress/sql/aggregates.sql
+++ b/src/test/regress/sql/aggregates.sql
@@ -452,6 +452,16 @@ select f1, (select distinct min(t1.f1) from int4_tbl t1
where t1.f1 = t0.f1)
from int4_tbl t0;
rollback;
+--Check min/max optimization for join expressions
+create unique index tenk1_unique1_idx on tenk1(unique1);
+create index tenk1_unique2_idx on tenk1(unique2);
+create unique index INT4_TBL_f1_idx on INT4_TBL(f1);
+explain (COSTS OFF)
+select min(t1.unique2) from tenk1 t1 left join INT4_TBL t2 on t1.unique1 =
t2.f1;
+DROP INDEX tenk1_unique1_idx;
+DROP INDEX tenk1_unique2_idx;
+DROP INDEX INT4_TBL_f1_idx;
+
-- check for correct detection of nested-aggregate errors
select max(min(unique1)) from tenk1;
select (select max(min(unique1)) from int8_tbl) from tenk1;
