From: Tom Lane <tgl@sss.pgh.pa.us>
To: Richard Guo <guofenglinux@gmail.com>
cc: Justin Pryzby <pryzby@telsasoft.com>, pgsql-hackers@lists.postgresql.org,
        "Finnerty,
    Jim" <jfinnert@amazon.com>
Subject: Re: Making Vars outer-join aware
In-reply-to: 
 <CAMbWs4-K8_yqARnrmRB5=xAWTJGgTpimQinvaa-k6c8f6Hideg@mail.gmail.com>
References: 
 <CAMbWs4-mvPPCJ1W6iK6dD5HiNwoJdi6mZp=-7mE8N9Sh+cd0tQ@mail.gmail.com>
 <CAMbWs4_WNcvQGiFMLJanj3h6WMzFi6P-=bifmnuXUcg4PSXQgA@mail.gmail.com>
 <1035990.1667261942@sss.pgh.pa.us> <2901865.1667685211@sss.pgh.pa.us>
 <CAMbWs4_pH7w7voY0AejUM4RiKq-Xuf+sXmoF8PcQwzNrsagEXA@mail.gmail.com>
 <CAMbWs4_gXgGdkkOraUPEoZD_Q3xTW7FTxfomW+fxPN2Kr-YSXg@mail.gmail.com>
 <4019771.1672155080@sss.pgh.pa.us>
 <CAMbWs4_JDDdXvEBWtTao9ZTwOGdDCU7X-=bF7yi9G1u_-b0Dfw@mail.gmail.com>
 <323812.1672241806@sss.pgh.pa.us> <2043562.1674506286@sss.pgh.pa.us>
 <20230212235823.GW1653@telsasoft.com>
 <CAMbWs4-K8_yqARnrmRB5=xAWTJGgTpimQinvaa-k6c8f6Hideg@mail.gmail.com>
Comments: In-reply-to Richard Guo <guofenglinux@gmail.com>
	message dated "Mon, 13 Feb 2023 15:33:15 +0800"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1223371.1676307017.1@sss.pgh.pa.us>
Date: Mon, 13 Feb 2023 11:50:17 -0500
Message-ID: <1223373.1676307017@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/1223373.1676307017%40sss.pgh.pa.us>
Precedence: bulk

Richard Guo <guofenglinux@gmail.com> writes:
> Thanks for the report!  I've looked at it a little bit and traced down
> to function have_unsafe_outer_join_ref().  The comment there says
>  * In practice, this test never finds a problem ...
> This seems not correct as showed by the counterexample.

Right.  I'd noticed that the inner loop of that function was not
reached in our regression tests, and incorrectly concluded that it
was not reachable --- but I failed to consider cases where the
inner rel's parameterization depends on Vars from multiple places.

> The NOT_USED part of code is doing this check.  But I think we need a
> little tweak.  We should check the nullable side of related outer joins
> against the satisfied part, rather than inner_paramrels.  Maybe
> something like attached.

Agreed.

> However, this test seems to cost some cycles after the change.  So I
> wonder if it's worthwhile to perform it, considering that join order
> restrictions should be able to guarantee there is no problem here.

Yeah, I think we should reduce it to an Assert check.  It shouldn't be
worth the cycles to run in production, and that will also make it easier
to notice in sqlsmith testing if anyone happens across another
counterexample.

Pushed that way.  Thanks for the report and the patch!

			regards, tom lane