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