On 21/2/2024 14:26, Richard Guo wrote:
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.
This code looks good. No idea how we have lost it before.
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.
Thanks, looks good.
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.
I see only one unusual query with the pg_class involved.
* 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.
I see uppercase -> lowercase change:
select t1.*, t2.a as ax from sj t1 join sj t2
and lowercase -> uppercase in many other cases:
explain (costs off)
I guess it is a matter of taste, so give up for the committer decision.
Technically, it's OK.
* 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.
I can't assess the necessity of changing these dozens of lines of code
because I follow another commenting style, but technically, it's still OK.
--
regards,
Andrei Lepikhov
Postgres Professional
