Re: Tracking per-RelOptInfo uniqueness during planning

On Thu, Jul 30, 2026 at 4:06 AM Antonin Houska <ah(at)cybertec(dot)at> wrote:
> Thanks for working on this. Following is my initial review.

Thanks for reviewing!

> What makes it a bit difficult for me to think about the logic is the
> 'nullable' field of the UniqueKey structure. I don't think the name is very
> descriptive. It seems to indicate that an OJ generated problematic NULL values
> that affect uniqueness of the output, however populate_plain_rel_uniquekeys()
> appears to set the sometimes as well.

Fair point. I didn't put much thought into the naming in v1. I've
renamed it to 'null_aware' in v2, which is a better name I believe.

> Another thing I don't understand is in get_interesting_unique_ecs():
>
> base_relids = bms_difference(ec->ec_relids, root->outer_join_rels);
>
> if (bms_membership(base_relids) == BMS_MULTIPLE)
> result = add_base_ec_positions(root, result, ec);
>
> Do we really need more than one non-nullable rels? Note that the function
> calls add_base_ec_positions(), for wich IMO a single non-nullable relation is
> sufficient:
>
> /* If ec is already a base EC, we add its own position */
> if (!bms_overlap(ec->ec_relids, root->outer_join_rels))
> return bms_add_member(result, ec->ec_index);

These are answering different questions. BMS_MULTIPLE decides which
ECs are worth tracking keys over; it isn't a precondition of
add_base_ec_positions(), which we do call with single-member ECs a bit
further down, from add_ojclause_ecs().

The reason we need the BMS_MULTIPLE check is that an EC covering one
base relation can't generate a join clause, so no consumer would ever
look it up through uniquekeys_match_join_clauses(). Single-relation
ECs still enter the set when another consumer can use them: the
DISTINCT columns, the GROUP BY columns, and both sides of a
mergejoinable outer-join clause. These are added later in that
function.

Removing the test wouldn't give wrong answers, but it would defeat the
point of interesting_unique_ecs, which is to bound how many keys we
track.

> Besides that, why do you create the ECs for unique keys in advance
> ("eagerly"), rather than creating them when actually needed ("lazily"? Is the
> reason that you only want to construct new ECs from the existing ones (because
> it's easier to retrieve the operator info), and thus the set of new ECs is
> does not depend on the uniqueness-related properties of relations?

Partly for the reason you guess. The more concrete reason is that
interesting_unique_ecs is a set of positions in root->eq_classes,
consumed while we build the base relations' keys. So the base EC must
already exist, and already be listed in rel->eclass_indexes.

> A few comments on coding:
>
> * in populate_subquery_rel_uniquekeys(), I think the test
>
> if (subquery->hasTargetSRFs)
> return;
>
> should appear at the very beginning, even before calling
> get_interesting_unique_ecs(), because it's very cheap.

Hmm, I don't think so. It guards only the set-operation branch
underneath it, not the whole function. A subquery with tlist SRFs can
still have unique keys to translate. "SELECT DISTINCT a,
generate_series(1,2) g FROM t" is distinct over (a, g): the DISTINCT
is applied above the SRF expansion, so the subquery's final relation
carries that key and we want it in the parent.

> * populate_joinrel_uniquekeys()
>
> /*
> * Preservation of the RHS keys, if the LHS is unique for the clauses.
> * Across a left join they become nullable, and an empty key does not
> * survive null-extension (one null-extended row per unmatched LHS row).
> */
> if (rhs->uniquekeys != NIL &&
> !bms_overlap(rhs->lateral_relids, lhs->relids) &&
> side_is_unique(root, joinrel, lhs, rhs, jointype, restrictlist))
> {
> foreach_node(UniqueKey, ukey, rhs->uniquekeys)
> {
> if (jointype == JOIN_INNER)
> add_uniquekey(joinrel, ukey->eclass_indexes, ukey->nullable);
> else if (!bms_is_empty(ukey->eclass_indexes))
> add_uniquekey(joinrel, ukey->eclass_indexes, true);
> }
> }
>
> 1. If jointype != JOIN_INNER, shouldn't we assert that it's JOIN_LEFT?

Yeah, we can have this Assert.

> 2. For the left join, should add_uniquekey() alwyas be called with
> nullable=true? If the LHS has uniquekeys at the same time, then the
> null-extended rows should be unique too, so the uniquekeys of the join might
> still be usable as a proof that the DISTINCT step is not necessary. (Of
> course, the 'nullable' attribute of the LHS uniquekeys matters in this case.)

The RHS key on its own isn't usable for that. Two unmatched LHS rows
produce two null-extended output rows whose RHS key columns are all
NULL, so they are equal on that key once NULLs count as equal. No LHS
property can change that, because the key doesn't mention the LHS.

What you're describing is the Combination rule a few lines below,
which adds the union of one key from each side, and that one does keep
its NULL-awareness across a left join.

> A comment explaining why 'subroot' can be NULL would make sense:
>
> if (subroot == NULL)
> return;

Added in v2.

> A few more minor suggestions are attached.

I absorbed into v2 some of your suggestions here. Thanks!

> A suggestion for future improvement (or have I missed that the patch already
> does that?): we can add unique keys to the grouping paths created due to eager
> aggregation.

Yeah, that's still in my to-do list, which also includes support for
appendrels, partitionwise child joins, UNION ALL parents, RTE_VALUES,
or RTE_RESULT, etc.

- Richard