On Fri, May 26, 2023 at 6:06 AM Tom Lane <t...@sss.pgh.pa.us> wrote:
> 1. The test case you give upthread only needs to ignore commute_below_l,
> that is it still passes without the lines
>
> + join_plus_commute = bms_add_members(join_plus_commute,
> +
> removable_sjinfo->commute_above_r);
>
> Based on what deconstruct_distribute_oj_quals is doing, it seems
> likely to me that there are cases that require ignoring
> commute_above_r, but I've not tried to devise one. It'd be good to
> have one to include in the commit, if we can find one.
It seems that queries of the second form of identity 3 require ignoring
commute_above_r.
select 1 from t t1 left join (t t2 left join t t3 on t2.a = t3.a) on
t1.a = t2.a;
When removing t2/t3 join, the clone of 't2.a = t3.a' with t1 join in the
nulling bitmap would be put back if we do not ignore commute_above_r.
There is no observable problem though because it would be rejected later
in subbuild_joinrel_restrictlist, but still I think it should not be put
back in the first place.
> 2. We could go a little further in refactoring, specifically the
> computation of joinrelids could be moved into remove_rel_from_query,
> since remove_useless_joins itself doesn't need it. Not sure if
> this'd be an improvement or not. Certainly it'd be a loser if
> remove_useless_joins grew a reason to need the value; but I can't
> foresee a reason for that to happen --- remove_rel_from_query is
> where basically all the work is going on.
+1 to move the computation of joinrelids into remove_rel_from_query().
We also do that in join_is_removable().
BTW, I doubt that the check of 'sjinfo->ojrelid != 0' in
remove_useless_joins() is needed. Since we've known that the join is
removable, I think we can just Assert that sjinfo->ojrelid cannot be 0.
--- a/src/backend/optimizer/plan/analyzejoins.c
+++ b/src/backend/optimizer/plan/analyzejoins.c
@@ -91,8 +91,8 @@ restart:
/* Compute the relid set for the join we are considering */
joinrelids = bms_union(sjinfo->min_lefthand, sjinfo->min_righthand);
- if (sjinfo->ojrelid != 0)
- joinrelids = bms_add_member(joinrelids, sjinfo->ojrelid);
+ Assert(sjinfo->ojrelid != 0);
+ joinrelids = bms_add_member(joinrelids, sjinfo->ojrelid);
remove_rel_from_query(root, innerrelid, sjinfo, joinrelids);
> 3. I called the parameter removable_sjinfo because sjinfo is already
> used within another loop, leading to a shadowed-parameter warning.
> In a green field we'd probably have called the parameter sjinfo
> and used another name for the loop's local variable, but that would
> make the patch bulkier without adding anything. Haven't decided
> whether to rename before commit or leave it as-is.
Personally I prefer to rename before commit but I'm OK with both ways.
Thanks
Richard
