Re: Tracking per-RelOptInfo uniqueness during planning

Richard Guo <guofenglinux(at)gmail(dot)com> wrote:

> This has been proposed several times over the years. The most recent
> and most developed attempts are the work by Andy Fan and David Rowley
> [1]. This patch takes a lot from that work and the discussion around
> it: the basic shape of a per-RelOptInfo list of unique keys, built
> bottom-up, with the keys expressed over ECs in the same spirit as
> pathkeys.

Thanks for working on this. Following is my initial review.

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.

I think the field name should rather give a hint about potential use of the
unique key, rather then telling how it was derived. Would something like
'null_equality' or 'consider_nulls_equal' make sense? In such a case, TRUE
value would then mean "the path output is unique even if NULL values are
considered equal".

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);

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?

I think that in the earlier versions of the patch, creation of the ECs was
driven by target expressions. This way we can generate more unique keys, but
construction of the corresponding ECs may be more difficult.

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.

* 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?

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.)

* translate_subquery_uniquekeys()

A comment explaining why 'subroot' can be NULL would make sense:

if (subroot == NULL)
return;

A few more minor suggestions are attached.

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.

--
Antonin Houska
Web: https://www.cybertec-postgresql.com