From 476b9eb3c4e136b62f0965408105d7c0e5b7dd4f Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 13 Oct 2026 17:53:20 -0400 Subject: [PATCH] Delay qp_callback processing till after join removal. Commit 2ebf25e7d left query_planner's qp_callback call where it had been historically, but that turns out to be a bad idea. It means that the code reached via qp_callback is subject to the must-be-able-to-restart rule created by looping back after join removal. We found one place that fails that requirement: adjust_group_pathkeys_for_groupagg sets Aggref.aggpresorted flags, and there's nothing that will reset those in case of a query_planner restart, yet the selected set of Aggrefs can change. There might be other places with similar issues. Fortunately, qp_callback's main charter is to compute/update pathkey values, and that is not something that join removal depends on or should depend on. (Whether join removal is valid is surely not dependent on what order the rel might get scanned in.) Nothing else within the restart loop depends on it either --- in other words, the existing placement was a bit random to begin with. So we can fix this problem just by moving that call to below the restart loop. That costs nothing, indeed it saves some cycles by not having to do that processing more than once. And removing an unstated requirement from code that's not directly called by query_planner should help forestall future bugs. While the known bug only exists in v18 and up (older branches lack the aggpresorted optimization), I've chosen to back-patch the code change to v16 like 2ebf25e7d was. This is the sort of API requirement that we don't really want to be different across branches. Reported-by: shihao zhong Author: Tom Lane Reviewed-by: shihao zhong Discussion: https://postgr.es/m/CAGRkXqSOAT1yop44B8jwacL_91rxK=QuoU8Of3NLMb9NgAO2NA@mail.gmail.com Backpatch-through: 16 --- src/backend/optimizer/plan/planmain.c | 14 +++++++------- src/test/regress/expected/join.out | 24 ++++++++++++++++++++++++ src/test/regress/sql/join.sql | 14 ++++++++++++++ 3 files changed, 45 insertions(+), 7 deletions(-) diff --git a/src/backend/optimizer/plan/planmain.c b/src/backend/optimizer/plan/planmain.c index f9831870142..e98e0e62004 100644 --- a/src/backend/optimizer/plan/planmain.c +++ b/src/backend/optimizer/plan/planmain.c @@ -257,13 +257,6 @@ restart: */ generate_base_implied_equalities(root); - /* - * We have completed merging equivalence sets, so it's now possible to - * generate pathkeys in canonical form; so compute query_pathkeys and - * other pathkeys fields in PlannerInfo. - */ - (*qp_callback) (root, qp_extra); - /* * Examine any "placeholder" expressions generated during subquery pullup. * Make sure that the Vars they need are marked as needed at the relevant @@ -302,6 +295,13 @@ restart: * this point does not need to be able to restart. */ + /* + * We have completed merging equivalence sets, so it's now possible to + * generate pathkeys in canonical form; so compute query_pathkeys and + * other pathkeys fields in PlannerInfo. + */ + (*qp_callback) (root, qp_extra); + /* * Now distribute "placeholders" to base rels as needed. This has to be * done after join removal because removal could change whether a diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out index 64a6773edec..e16646f888d 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -8883,6 +8883,30 @@ ON sj_t1.id = _t2t3t4.id; -> Seq Scan on sj_t1 (24 rows) +-- Check SJE interaction with aggpresorted optimization. +CREATE TEMP TABLE sje_agg (id int PRIMARY KEY, p int, q int); +INSERT INTO sje_agg SELECT i, i % 3, (i * 7) % 10 FROM generate_series(1, 30) i; +ANALYZE sje_agg; +EXPLAIN (COSTS OFF) +SELECT count(DISTINCT a.p) AS dp, count(DISTINCT a.q) AS dq, + sum(DISTINCT a2.q) AS sq +FROM sje_agg a JOIN sje_agg a2 ON a.id = a2.id; + QUERY PLAN +------------------------------------ + Aggregate + -> Sort + Sort Key: a2.q + -> Seq Scan on sje_agg a2 +(4 rows) + +SELECT count(DISTINCT a.p) AS dp, count(DISTINCT a.q) AS dq, + sum(DISTINCT a2.q) AS sq +FROM sje_agg a JOIN sje_agg a2 ON a.id = a2.id; + dp | dq | sq +----+----+---- + 3 | 10 | 45 +(1 row) + -- -- Test RowMarks-related code -- diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql index 1bd6741ee7f..27c9deaaf8f 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -3462,6 +3462,20 @@ JOIN ( ) _t2t3t4 ON sj_t1.id = _t2t3t4.id; +-- Check SJE interaction with aggpresorted optimization. +CREATE TEMP TABLE sje_agg (id int PRIMARY KEY, p int, q int); +INSERT INTO sje_agg SELECT i, i % 3, (i * 7) % 10 FROM generate_series(1, 30) i; +ANALYZE sje_agg; + +EXPLAIN (COSTS OFF) +SELECT count(DISTINCT a.p) AS dp, count(DISTINCT a.q) AS dq, + sum(DISTINCT a2.q) AS sq +FROM sje_agg a JOIN sje_agg a2 ON a.id = a2.id; + +SELECT count(DISTINCT a.p) AS dp, count(DISTINCT a.q) AS dq, + sum(DISTINCT a2.q) AS sq +FROM sje_agg a JOIN sje_agg a2 ON a.id = a2.id; + -- -- Test RowMarks-related code --