On Tue, Nov 28, 2023 at 1:42 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> :phnullingrels (b 5)
> It's correct to move both phrels locations to rel 7 (the surviving
> self-joined rel) but what became of the reference to nullingrel 5?
> That seems clearly wrong, since we have not removed the left join.
I must confess I couldn't reproduce this issue. I watched only the value
':phnullingrels (b)' everywhere.
On 5/12/2023 09:35, Richard Guo wrote:
>
> On Tue, Nov 28, 2023 at 3:03 PM Richard Guo <guofenglinux@gmail.com
> <mailto:guofenglinux@gmail.com>> wrote:
>
> On Tue, Nov 28, 2023 at 1:42 AM Tom Lane <tgl@sss.pgh.pa.us
> <mailto:tgl@sss.pgh.pa.us>> wrote:
>
> Much worse, if you look around elsewhere in the data structure,
> particularly at the processed_tlist, you find instances of the
> PlaceHolderVars that have not been updated and still have the
> old values such as ":phrels (b 6)".
>
>
> Good catch! We fail to handle PlaceHolderVar case in sje_walker and
> replace_varno_walker, which is wrong. Fixed in v3 patch.
Yes, it was my blunder. I didn't recognize that phinfo->ph_var points to
a copy of the PlaceHolderVar and thought it was enough to pass through
the root->placeholder_list only. Adding replacement code into the walker
is the proper correction.
> I noticed that this issue exists on master even without the
> Don't-constrain-SJE-due-to-PHVs patch, and it can be illustrated with a
> query from regression test.
>
> -- Test that placeholders are updated correctly after join removal
> explain (costs off)
> select * from (values (1)) x
> left join (select coalesce(y.q1, 1) from int8_tbl y
> right join sj j1 inner join sj j2 on j1.a = j2.a
> on true) z
> on true;
> Also, it seems to me that sje_walker is redundant. Currently it is used
> to walk the query tree for varno replacement, a task that can be
> fulfilled with replace_varno_walker.
Agree.
>> It's not apparent to me why the cross-checks we have in setrefs.c
>> and elsewhere don't detect a problem with this. But it seems
>> clear that remove_useless_self_joins is still several bricks
>> shy of a load in terms of fully updating the data structure for a
>> join removal. Probably with some more work to add complication
>> to this test case, we could demonstrate an observable failure.
> Fair point. I have the same concern that we still have other loose >
ends
> in SJE updating necessary data structures in self join removal. And I
> noticed that Andres expressed the same concern in [1]. I think we
> should come up with some solution to detect/avoid such problems, but I
> imagine that that should be a separate patch.
It is an excellent task to ponder. At least, we should answer later why
the setrefs.c didn't detect that, and could we do something here. Maybe
all we could do - is add comments to the PlannerInfo structure.
According to the patches:
0001 - looks good for me.
0002 - I don't understand why to use 'explain' in VERBOSE mode in tests.
What do you try to detect here?
0003 - ok, but too short. Maybe squash all these patches into one?
I think these corrections are good enough for commit.
--
regards,
Andrei Lepikhov
Postgres Professional
