Re: Removing unneeded self joins

On Mon, Feb 19, 2024 at 1:24 PM Andrei Lepikhov <a.lepikhov@postgrespro.ru>
wrote:

> On 18/2/2024 23:18, Alexander Korotkov wrote:
> > On Sun, Feb 18, 2024 at 5:04 PM Alexander Korotkov <aekorotkov@gmail.com>
> wrote:
> >> On Sun, Feb 18, 2024 at 3:00 PM Alexander Lakhin <exclusion@gmail.com>
> wrote:
> >>> Please look at the following query which fails with an error since
> >>> d3d55ce57:
> >>>
> >>> create table t (i int primary key);
> >>>
> >>> select t3.i from t t1
> >>>   join t t2 on t1.i = t2.i,
> >>>   lateral (select t1.i limit 1) t3;
> >>>
> >>> ERROR:  non-LATERAL parameter required by subquery
> >>
> >> Thank you for spotting.  I'm looking at this.
> >
> > Attached is a draft patch fixing this query.  Could you, please, recheck?
> I reviewed this patch. Why do you check only the target list? I guess
> these links can be everywhere. See the patch in the attachment with the
> elaborated test and slightly changed code.


I just noticed that this fix has been committed in 489072ab7a, but it's
just flat wrong.

* The fix walks the subquery and replaces all the Vars with a varno
equal to the relid of the removing rel, without checking the
varlevelsup.  That is to say, a Var that belongs to the subquery itself
might also be replaced, which is wrong.  For instance,

create table t (i int primary key);

explain (costs off)
select t3.i from t t1
  join t t2 on t1.i = t2.i
  join lateral (select * from (select t1.i offset 0) offset 0) t3 on true;
ERROR:  no relation entry for relid 2


* The fix only traverses one level within the subquery, so Vars that
appear in subqueries with multiple levels cannot be replaced.  For
instance,

explain (costs off)
select t4.i from t t1
  join t t2 on t1.i = t2.i
  join lateral (select t3.i from t t3, (select t1.i) offset 0) t4 on true;
ERROR:  non-LATERAL parameter required by subquery


I think the right fix for these issues is to introduce a new element
'sublevels_up' in ReplaceVarnoContext, and enhance replace_varno_walker
to 1) recurse into subselects with sublevels_up increased, and 2)
perform the replacement only when varlevelsup is equal to sublevels_up.

Attached is a patch for the fix.

While writing the fix, I noticed some outdated comments.  Such as in
remove_rel_from_query, the first for loop updates otherrel's attr_needed
as well as lateral_vars, but the comment only mentions attr_needed.  So
this patch also fixes some outdated comments.

While writing the test cases, I found that the test cases for SJE are
quite messy.  Below are what I have noticed:

* There are several test cases using catalog tables like pg_class,
pg_stats, pg_index, etc. for testing join removal.  I don't see a reason
why we need to use catalog tables, and I think this just raises the risk
of instability.

* In many test cases, a mix of uppercase and lowercase keywords is used
in one query.  I think it'd better to maintain consistency by using
either all uppercase or all lowercase keywords in one query.

* In most situations, we verify the plan and the output of a query like:

explain (costs off)
select ...;
select ...;

The two select queries are supposed to be the same.  But in the SJE test
cases, I have noticed instances where the two select queries differ from
each other.

This patch also includes some cosmetic tweaks for SJE test cases.  It
does not change the test cases using catalog tables though.  I think
that should be a seperate patch.

Thanks
Richard