Subject: Re: Inconsistent nullingrels due to oversight in deconstruct_distribute_oj_quals

From: Richard Guo <[email-masked]>
Date: Fri, 10 Feb 2023 16:27:14 +0800

On Fri, Feb 10, 2023 at 11:08 AM Richard Guo <[email-masked]> wrote:

> On Thu, Feb 9, 2023 at 11:55 PM Tom Lane <[email-masked]> wrote:
>
>> Richard Guo <[email-masked]> writes:
>> > This query would trigger the Assert() in search_indexed_tlist_for_var.
>> > So I wonder that we should use othersj->syn_righthand here.
>>
>> There are two such calls in deconstruct_distribute_oj_quals ...
>> don't they both need this change?
>
>
> Yeah, I wondered about that too, but didn't manage to devise a query
> that can show the problem caused by the call for 'above_sjinfo' case.
> After a night of sleep I came up with one this morning. :-)
>
> create table t (a int, b int);
>
> insert into t select i, i from generate_series(1,10)i;
> analyze t;
>
> select * from t t1 left join t t2 left join t t3 on t2.b = t3.b left join
> t t4 on t2.a > t3.a on t2.a > t1.a;
>
> In this query, for the qual 't2.a > t3.a', when we try to push t3/t4
> join to above t1/t2 join, we fail to add t1/t2 ojrelid to
> nullingrels of t3.a, because t3 is not in t1/t2 join's min_righthand
> (but in its syn_righthand).  We really should have done that because
> after the commutation t1/t2 join can null not only t2 but also t3 in
> this case.
>

However, for 'above_sjinfo' case, we should not use
othersj->syn_righthand,  because othersj->syn_righthand contains relids
in sjinfo's righthand which should not be nulled by othersj after the
commutation.  It seems what we should use here is sjinfo->syn_lefthand.

--- a/src/backend/optimizer/plan/initsplan.c
+++ b/src/backend/optimizer/plan/initsplan.c
@@ -1990,7 +1990,7 @@ deconstruct_distribute_oj_quals(PlannerInfo *root,
        if (above_sjinfo)
            quals = (List *)
                add_nulling_relids((Node *) quals,
-                                  othersj->min_righthand,
+                                  sjinfo->syn_lefthand,
                                   bms_make_singleton(othersj->ojrelid));

Thanks
Richard

---

From: Tom Lane <[email-masked]>
Date: Fri, 10 Feb 2023 13:40:30 -0500

> commutation.  It seems what we should use here is sjinfo->syn_lefthand.

I had a hard time wrapping my brain around that to start with, but
now I think you're right.  othersj is syntactically above the current
join, so its syn_righthand will cover all of the current join, but
we only want to add nulling bits to Vars of the current join's LHS.
(That is, we need to transform Pbc to Pb*c, not Pb*c*.)

I also realized that there was a fairly critical nearby bug:
make_outerjoininfo was failing to check whether the upper join's qual
is actually of the form "Pbc", without any references to the lower join's
LHS.  So that led us to setting commute bits in some cases where we
shouldn't, further confusing deconstruct_distribute_oj_quals.  (I think
this snuck in because its other code path doesn't need to make such a
check, it being syntactically impossible to have such a reference if
we start from the other form of the identity.)

Fix pushed.  This seems to take care of Robins' latest example in
the bug #17781 thread [1], too.

			regards, tom lane

[1] https://postgr.es/m/[email-masked]