From db2f9afe6548bc4eb02b876e8f1d788f90264538 Mon Sep 17 00:00:00 2001 From: Evdokimov Ilia Date: Thu, 23 Jul 2026 16:45:40 +0300 Subject: [PATCH v2 9/9] Populate uniquekeys for GROUP BY, so it can make DISTINCT redundant Each output row of GROUP BY already corresponds to exactly one group, so a DISTINCT clause covering all the GROUP BY keys is pure overhead. Wire up populate_uniquekeys_from_pathkeys() (goal 4 of the design doc, previously a commented-out stub) to record this on grouped_rel, and let the existing relation_is_distinct_for()/create_distinct_paths() machinery pick it up for free, same as it already does for unique indexes and joins. Since a grouped_rel is an upper rel, UniqueKey.item_indexes has to be target-list positions rather than root->eq_classes positions. relation_is_distinct_for() had only ever seen base/join rels and built its comparison bitmapset as eq_classes positions unconditionally; the mismatch was latent until an upper rel actually got real uniquekeys, surfacing as wrong results on postgres_fdw (duplicate rows on a DISTINCT+GROUP BY query). Fixed by branching on IS_UPPER_REL() and adding find_ec_target_position(), a shared EC-to-target-position lookup that matches any EC member expression (not just Vars, unlike the similar lookup in convert_unique_keys_for_rel()) -- needed because an aggregate like sum(c) is its own EC with only an Aggref member. Grouping also collapses every NULL-valued row of a column into one output row, unlike a unique index, which permits multiple NULLs. Added a no_multinulls flag on UniqueKey, set for GROUP BY-derived keys, so uniquekey_contains_multinulls() skips its nullability check for them. The SRF and opclass/collation edge cases both turn out to already be covered by existing machinery rather than needing new guards: SRFs via convert_unique_keys_for_rel()'s existing hasTargetSRFs check, and opclass/collation because group_pathkeys and distinct_pathkeys only share a canonical EC when their operators and collations already agree. --- src/backend/optimizer/path/uniquekey.c | 110 ++++++++++- src/backend/optimizer/plan/planner.c | 6 +- src/backend/optimizer/util/plancat.c | 15 +- src/include/nodes/pathnodes.h | 11 ++ src/include/optimizer/paths.h | 2 + src/test/regress/expected/select_distinct.out | 172 ++++++++++++++++++ src/test/regress/sql/select_distinct.sql | 73 ++++++++ 7 files changed, 379 insertions(+), 10 deletions(-) diff --git a/src/backend/optimizer/path/uniquekey.c b/src/backend/optimizer/path/uniquekey.c index adebe43c889..2e281dc2150 100644 --- a/src/backend/optimizer/path/uniquekey.c +++ b/src/backend/optimizer/path/uniquekey.c @@ -60,6 +60,7 @@ static void print_uniquekey(PlannerInfo *root, RelOptInfo *rel); static bool uniquekey_contains_in(PlannerInfo *root, UniqueKey * ukey, Bitmapset *ecs, Relids relids); static bool is_uniquekey_useful_afterjoin(PlannerInfo *root, UniqueKey * ukey, RelOptInfo *joinrel); static int find_expr_pos_in_list(Expr *expr, List *list); +static int find_ec_target_position(EquivalenceClass *ec, List *target_exprs); /* * populate_baserel_uniquekeys @@ -479,6 +480,9 @@ uniquekey_contains_multinulls(PlannerInfo *root, RelOptInfo *rel, UniqueKey * uk { int i = -1; + if (ukey->no_multinulls) + return false; + while ((i = bms_next_member(ukey->item_indexes, i)) >= 0) { EquivalenceClass *ec = list_nth_node(EquivalenceClass, root->eq_classes, i); @@ -511,6 +515,43 @@ uniquekey_contains_multinulls(PlannerInfo *root, RelOptInfo *rel, UniqueKey * uk } +/* + * find_ec_target_position + * + * Find a member of 'ec' that also appears (per equal()) in + * 'target_exprs', and return its position there, or -1 if none of ec's + * members can be matched. This is how an EquivalenceClass (root->eq_classes + * position) is translated into an upper rel's target-position indexing (see + * the UniqueKey.item_indexes comment). + * + * Unlike convert_unique_keys_for_rel(), which only accepts plain-Var + * members when building a *new* uniquekey's item_indexes, this is used to + * locate arbitrary distinct_pathkeys columns (e.g. an aggregate result + * like sum(c) can legitimately be its own EC with only an Aggref member), + * so any member expression is accepted, not just Vars. + */ +static int +find_ec_target_position(EquivalenceClass *ec, List *target_exprs) +{ + ListCell *lc; + + foreach(lc, ec->ec_members) + { + EquivalenceMember *em = lfirst_node(EquivalenceMember, lc); + Expr *expr = em->em_expr; + int pos; + + /* EMs can be wrapped in RelabelType. */ + while (expr && IsA(expr, RelabelType)) + expr = ((RelabelType *) expr)->arg; + + pos = find_expr_pos_in_list(expr, target_exprs); + if (pos >= 0) + return pos; + } + return -1; +} + /* * relation_is_distinct_for * @@ -528,10 +569,22 @@ relation_is_distinct_for(PlannerInfo *root, RelOptInfo *rel, List *distinct_path return !uniquekey_contains_multinulls(root, rel, singlerow_ukey); } + /* + * rel->uniquekeys' item_indexes are root->eq_classes positions for a + * base/join rel, but target-list positions for an upper rel (e.g. a + * grouped_rel) -- see the UniqueKey.item_indexes comment. Build + * pathkey_bm using whichever indexing rel itself uses, or the + * bms_is_subset() check below compares two incompatible numberings. + */ foreach(lc, distinct_pathkey) { PathKey *pathkey = lfirst_node(PathKey, lc); - int pos = list_member_ptr_pos(root->eq_classes, pathkey->pk_eclass); + int pos; + + if (IS_UPPER_REL(rel)) + pos = find_ec_target_position(pathkey->pk_eclass, rel->reltarget->exprs); + else + pos = list_member_ptr_pos(root->eq_classes, pathkey->pk_eclass); if (pos == -1) return false; @@ -1169,6 +1222,61 @@ create_uniquekeys_for_sortop(SetOperationStmt *topop, List *targetlist) return list_make1(key); } +/* + * populate_uniquekeys_from_pathkeys + * + * Record that 'rel' (an upper rel, e.g. a grouped_rel) is unique on + * 'pathkeys' (e.g. the GROUP BY keys). Since 'rel' is an upper rel, the + * resulting key's item_indexes must be positions in rel->reltarget->exprs + * (per the convention documented on UniqueKey.item_indexes), not + * root->eq_classes positions -- so for each pathkey's EquivalenceClass we + * look for a plain Var member that also appears in rel->reltarget->exprs, + * mirroring the EC-to-target-position matching convert_unique_keys_for_rel() + * does when converting a base/join rel's key for an upper rel. If any + * pathkey can't be matched this way, give up on the whole key rather than + * recording a partial/incorrect one. + * + * Unlike a unique index, grouping guarantees at most one output row per + * distinct key value even when that value is NULL (NULLs are grouped + * together), so the resulting key is marked no_multinulls and + * uniquekey_contains_multinulls() will not second-guess it based on the + * nullability of the underlying columns. + */ +void +populate_uniquekeys_from_pathkeys(PlannerInfo *root, RelOptInfo *rel, + List *pathkeys) +{ + Bitmapset *item_indexes = NULL; + List *opfamily_lists = NIL; + ListCell *lc; + UniqueKey *ukey; + + if (pathkeys == NIL) + return; + + foreach(lc, pathkeys) + { + PathKey *pathkey = lfirst_node(PathKey, lc); + EquivalenceClass *ec = pathkey->pk_eclass; + int matched; + + matched = find_ec_target_position(ec, rel->reltarget->exprs); + if (matched < 0) + { + bms_free(item_indexes); + list_free(opfamily_lists); + return; + } + + item_indexes = bms_add_member(item_indexes, matched); + opfamily_lists = lappend(opfamily_lists, ec->ec_opfamilies); + } + + ukey = make_uniquekey(item_indexes, opfamily_lists, true); + ukey->no_multinulls = true; + rel->uniquekeys = lappend(rel->uniquekeys, ukey); +} + /* * Return a zero-based index of an expression in a list, or -1 if not found. */ diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index 7dc91f63aed..d6c8be27190 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -4464,10 +4464,8 @@ create_ordinary_grouping_paths(PlannerInfo *root, RelOptInfo *input_rel, } else if (root->parse->groupClause && root->group_pathkeys != NIL) { - /* - * populate_uniquekeys_from_pathkeys(root, grouped_rel, - * root->group_pathkeys); - */ + populate_uniquekeys_from_pathkeys(root, grouped_rel, + root->group_pathkeys); } else { diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c index ea708e268de..c55dee7637c 100644 --- a/src/backend/optimizer/util/plancat.c +++ b/src/backend/optimizer/util/plancat.c @@ -125,7 +125,6 @@ get_relation_info(PlannerInfo *root, Oid relationObjectId, bool inhparent, Relation relation; bool hasindex; List *indexinfos = NIL; - Index i; /* * We need not lock the relation since it was already locked, either by @@ -192,13 +191,19 @@ get_relation_info(PlannerInfo *root, Oid relationObjectId, bool inhparent, /* Retrieve the parallel_workers reloption, or -1 if not set. */ rel->rel_parallel_workers = RelationGetParallelWorkers(relation, -1); - for (i = 0; i < relation->rd_att->natts; i++) + /* + * Seed notnullattrs from notnullattnums (populated above), converting + * between the two Bitmapsets' encodings, instead of re-scanning + * pg_attribute here. Unlike notnullattnums, notnullattrs is also + * augmented elsewhere (e.g. with subquery-derived not-null facts), so + * it has to remain its own Bitmapset rather than just an alias. + */ { - Form_pg_attribute attr = TupleDescAttr(relation->rd_att, i); + int attnum = -1; - if (attr->attnotnull) + while ((attnum = bms_next_member(rel->notnullattnums, attnum)) >= 0) rel->notnullattrs = bms_add_member(rel->notnullattrs, - attr->attnum - FirstLowInvalidHeapAttributeNumber); + attnum - FirstLowInvalidHeapAttributeNumber); } /* diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h index 3e975e4ec93..f1731f94251 100644 --- a/src/include/nodes/pathnodes.h +++ b/src/include/nodes/pathnodes.h @@ -1872,6 +1872,17 @@ typedef struct UniqueKey bool use_for_distinct; /* true if it is used in distinct-pathkey, * in this case we would never check if we * should discard it during join search. */ + + /* + * True if the key's uniqueness already accounts for NULLs, i.e. it is + * known that no two rows can agree on NULL for these columns, so + * uniquekey_contains_multinulls() need not inspect the underlying + * columns' nullability. This holds for keys derived from GROUP BY + * (grouping collapses all NULL-valued rows of a column into a single + * output row), unlike keys derived from a unique index (which permits + * multiple NULLs). + */ + bool no_multinulls; } UniqueKey; diff --git a/src/include/optimizer/paths.h b/src/include/optimizer/paths.h index b101155c173..0e2e3033441 100644 --- a/src/include/optimizer/paths.h +++ b/src/include/optimizer/paths.h @@ -301,6 +301,8 @@ extern void convert_unique_keys_for_rel(PlannerInfo *root, RelOptInfo *rel, PathTarget *input_target); extern List *create_uniquekeys_for_sortop(SetOperationStmt *topop, List *targetlist); +extern void populate_uniquekeys_from_pathkeys(PlannerInfo *root, + RelOptInfo *rel, List *pathkeys); extern bool uniquekey_contains_multinulls(PlannerInfo *root, RelOptInfo *rel, UniqueKey * ukey); #endif /* PATHS_H */ diff --git a/src/test/regress/expected/select_distinct.out b/src/test/regress/expected/select_distinct.out index 741f7238742..acff24748e5 100644 --- a/src/test/regress/expected/select_distinct.out +++ b/src/test/regress/expected/select_distinct.out @@ -596,3 +596,175 @@ SELECT DISTINCT y, x FROM distinct_tbl ORDER BY y; RESET enable_hashagg; DROP TABLE distinct_tbl; +-- +-- Test that DISTINCT is removed when GROUP BY already guarantees uniqueness +-- of the output rows (via populate_uniquekeys_from_pathkeys() feeding the +-- generic relation_is_distinct_for() check in create_distinct_paths()). +-- +CREATE TABLE distinct_groupby_tbl (a int, b int, c int); +INSERT INTO distinct_groupby_tbl VALUES + (1, 1, 10), (1, 2, 20), (2, 1, 30), (2, 2, 40), + (1, 1, 50); +ANALYZE distinct_groupby_tbl; +-- GROUP BY (a, b) guarantees unique output rows, so DISTINCT is redundant. +-- Expect a single aggregation node, no extra Unique node on top. +EXPLAIN (COSTS OFF) +SELECT DISTINCT a, b FROM distinct_groupby_tbl GROUP BY a, b; + QUERY PLAN +---------------------------------------- + HashAggregate + Group Key: a, b + -> Seq Scan on distinct_groupby_tbl +(3 rows) + +-- Verify correct results +SELECT DISTINCT a, b FROM distinct_groupby_tbl GROUP BY a, b ORDER BY a, b; + a | b +---+--- + 1 | 1 + 1 | 2 + 2 | 1 + 2 | 2 +(4 rows) + +-- Different column order in DISTINCT vs GROUP BY -- still redundant. +EXPLAIN (COSTS OFF) +SELECT DISTINCT b, a FROM distinct_groupby_tbl GROUP BY a, b; + QUERY PLAN +---------------------------------------- + HashAggregate + Group Key: a, b + -> Seq Scan on distinct_groupby_tbl +(3 rows) + +SELECT DISTINCT b, a FROM distinct_groupby_tbl GROUP BY a, b ORDER BY a, b; + b | a +---+--- + 1 | 1 + 2 | 1 + 1 | 2 + 2 | 2 +(4 rows) + +-- Aggregate in SELECT list does not prevent elimination. +EXPLAIN (COSTS OFF) +SELECT DISTINCT a, b, sum(c) FROM distinct_groupby_tbl GROUP BY a, b; + QUERY PLAN +---------------------------------------- + HashAggregate + Group Key: a, b + -> Seq Scan on distinct_groupby_tbl +(3 rows) + +SELECT DISTINCT a, b, sum(c) FROM distinct_groupby_tbl GROUP BY a, b ORDER BY a, b; + a | b | sum +---+---+----- + 1 | 1 | 60 + 1 | 2 | 20 + 2 | 1 | 30 + 2 | 2 | 40 +(4 rows) + +-- DISTINCT is NOT redundant: GROUP BY key 'b' is absent from DISTINCT. +-- Different (a, b) groups can produce the same 'a' output value. +EXPLAIN (COSTS OFF) +SELECT DISTINCT a FROM distinct_groupby_tbl GROUP BY a, b; + QUERY PLAN +---------------------------------------------------- + Unique + -> Sort + Sort Key: a + -> HashAggregate + Group Key: a, b + -> Seq Scan on distinct_groupby_tbl +(6 rows) + +SELECT DISTINCT a FROM distinct_groupby_tbl GROUP BY a, b ORDER BY a; + a +--- + 1 + 2 +(2 rows) + +-- DISTINCT is NOT redundant: no GROUP BY clause. +EXPLAIN (COSTS OFF) +SELECT DISTINCT a, b FROM distinct_groupby_tbl; + QUERY PLAN +---------------------------------------- + HashAggregate + Group Key: a, b + -> Seq Scan on distinct_groupby_tbl +(3 rows) + +-- DISTINCT is NOT redundant: GROUPING SETS can introduce extra NULL rows, +-- so two grouping sets could yield the same DISTINCT output. +EXPLAIN (COSTS OFF) +SELECT DISTINCT a FROM distinct_groupby_tbl +GROUP BY GROUPING SETS ((a), ()); + QUERY PLAN +---------------------------------------------- + HashAggregate + Group Key: a + -> MixedAggregate + Hash Key: a + Group Key: () + -> Seq Scan on distinct_groupby_tbl +(6 rows) + +-- DISTINCT ON is unaffected (different semantics). +EXPLAIN (COSTS OFF) +SELECT DISTINCT ON (a) a, b FROM distinct_groupby_tbl GROUP BY a, b ORDER BY a; + QUERY PLAN +---------------------------------------------------- + Unique + -> Sort + Sort Key: a + -> HashAggregate + Group Key: a, b + -> Seq Scan on distinct_groupby_tbl +(6 rows) + +-- DISTINCT is NOT redundant: a set-returning function in the targetlist is +-- expanded after grouping but before DISTINCT, so it can (re-)introduce +-- duplicate rows even though every GROUP BY key is covered by DISTINCT. +EXPLAIN (COSTS OFF) +SELECT DISTINCT a, unnest(ARRAY[1,1]) AS u FROM distinct_groupby_tbl GROUP BY a; + QUERY PLAN +---------------------------------------------------- + HashAggregate + Group Key: a, unnest('{1,1}'::integer[]) + -> ProjectSet + -> HashAggregate + Group Key: a + -> Seq Scan on distinct_groupby_tbl +(6 rows) + +SELECT DISTINCT a, unnest(ARRAY[1,1]) AS u FROM distinct_groupby_tbl GROUP BY a ORDER BY a, u; + a | u +---+--- + 1 | 1 + 2 | 1 +(2 rows) + +-- DISTINCT is still redundant when the grouped column is nullable: GROUP BY +-- collapses all NULL-valued rows of a column into a single output row, so a +-- NULL group is no more of a duplicate risk than any other group value. +INSERT INTO distinct_groupby_tbl VALUES (NULL, 1, 60), (NULL, 1, 70); +EXPLAIN (COSTS OFF) +SELECT DISTINCT a FROM distinct_groupby_tbl GROUP BY a; + QUERY PLAN +---------------------------------------- + HashAggregate + Group Key: a + -> Seq Scan on distinct_groupby_tbl +(3 rows) + +SELECT DISTINCT a FROM distinct_groupby_tbl GROUP BY a ORDER BY a; + a +--- + 1 + 2 + +(3 rows) + +DROP TABLE distinct_groupby_tbl; diff --git a/src/test/regress/sql/select_distinct.sql b/src/test/regress/sql/select_distinct.sql index 2ed1616b098..69c83fe8e1a 100644 --- a/src/test/regress/sql/select_distinct.sql +++ b/src/test/regress/sql/select_distinct.sql @@ -274,3 +274,76 @@ SELECT DISTINCT y, x FROM distinct_tbl ORDER BY y; RESET enable_hashagg; DROP TABLE distinct_tbl; + +-- +-- Test that DISTINCT is removed when GROUP BY already guarantees uniqueness +-- of the output rows (via populate_uniquekeys_from_pathkeys() feeding the +-- generic relation_is_distinct_for() check in create_distinct_paths()). +-- + +CREATE TABLE distinct_groupby_tbl (a int, b int, c int); +INSERT INTO distinct_groupby_tbl VALUES + (1, 1, 10), (1, 2, 20), (2, 1, 30), (2, 2, 40), + (1, 1, 50); +ANALYZE distinct_groupby_tbl; + +-- GROUP BY (a, b) guarantees unique output rows, so DISTINCT is redundant. +-- Expect a single aggregation node, no extra Unique node on top. +EXPLAIN (COSTS OFF) +SELECT DISTINCT a, b FROM distinct_groupby_tbl GROUP BY a, b; + +-- Verify correct results +SELECT DISTINCT a, b FROM distinct_groupby_tbl GROUP BY a, b ORDER BY a, b; + +-- Different column order in DISTINCT vs GROUP BY -- still redundant. +EXPLAIN (COSTS OFF) +SELECT DISTINCT b, a FROM distinct_groupby_tbl GROUP BY a, b; + +SELECT DISTINCT b, a FROM distinct_groupby_tbl GROUP BY a, b ORDER BY a, b; + +-- Aggregate in SELECT list does not prevent elimination. +EXPLAIN (COSTS OFF) +SELECT DISTINCT a, b, sum(c) FROM distinct_groupby_tbl GROUP BY a, b; + +SELECT DISTINCT a, b, sum(c) FROM distinct_groupby_tbl GROUP BY a, b ORDER BY a, b; + +-- DISTINCT is NOT redundant: GROUP BY key 'b' is absent from DISTINCT. +-- Different (a, b) groups can produce the same 'a' output value. +EXPLAIN (COSTS OFF) +SELECT DISTINCT a FROM distinct_groupby_tbl GROUP BY a, b; + +SELECT DISTINCT a FROM distinct_groupby_tbl GROUP BY a, b ORDER BY a; + +-- DISTINCT is NOT redundant: no GROUP BY clause. +EXPLAIN (COSTS OFF) +SELECT DISTINCT a, b FROM distinct_groupby_tbl; + +-- DISTINCT is NOT redundant: GROUPING SETS can introduce extra NULL rows, +-- so two grouping sets could yield the same DISTINCT output. +EXPLAIN (COSTS OFF) +SELECT DISTINCT a FROM distinct_groupby_tbl +GROUP BY GROUPING SETS ((a), ()); + +-- DISTINCT ON is unaffected (different semantics). +EXPLAIN (COSTS OFF) +SELECT DISTINCT ON (a) a, b FROM distinct_groupby_tbl GROUP BY a, b ORDER BY a; + +-- DISTINCT is NOT redundant: a set-returning function in the targetlist is +-- expanded after grouping but before DISTINCT, so it can (re-)introduce +-- duplicate rows even though every GROUP BY key is covered by DISTINCT. +EXPLAIN (COSTS OFF) +SELECT DISTINCT a, unnest(ARRAY[1,1]) AS u FROM distinct_groupby_tbl GROUP BY a; + +SELECT DISTINCT a, unnest(ARRAY[1,1]) AS u FROM distinct_groupby_tbl GROUP BY a ORDER BY a, u; + +-- DISTINCT is still redundant when the grouped column is nullable: GROUP BY +-- collapses all NULL-valued rows of a column into a single output row, so a +-- NULL group is no more of a duplicate risk than any other group value. +INSERT INTO distinct_groupby_tbl VALUES (NULL, 1, 60), (NULL, 1, 70); + +EXPLAIN (COSTS OFF) +SELECT DISTINCT a FROM distinct_groupby_tbl GROUP BY a; + +SELECT DISTINCT a FROM distinct_groupby_tbl GROUP BY a ORDER BY a; + +DROP TABLE distinct_groupby_tbl; -- 2.34.1