Horton

Re: Removing unneeded self joins

Andrei Lepikhov <a.lepikhov@postgrespro.ru>

View thread
From: Andrei Lepikhov <a.lepikhov@postgrespro.ru>
To: Richard Guo <guofenglinux@gmail.com>
Cc: Alexander Korotkov <aekorotkov@gmail.com>, Alexander Lakhin <exclusion@gmail.com>, "Gregory Stark (as CFM)" <stark.cfm@gmail.com>, Michał Kłeczek <michal@kleczek.org>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Tom Lane <tgl@sss.pgh.pa.us>
Date: 2024-02-22T08:51:19Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Remove GUC_NOT_IN_SAMPLE from enable_self_join_elimination

  2. Put enable_self_join_elimination into postgresql.conf.sample

  3. Get rid of ojrelid local variable in remove_rel_from_query()

  4. Implement Self-Join Elimination

  5. Revert: Remove useless self-joins

  6. Replace lateral references to removed rels in subqueries

  7. Replace relids in lateral subquery parse tree during SJE

  8. Forbid SJE with result relation

  9. Fix misuse of RelOptInfo.unique_for_rels cache by SJE

  10. Replace the relid in some missing fields during SJE

  11. Revert 56-bit relfilenode change and follow-up commits.

  12. Stabilize timetz test across DST transitions.

  13. Speed up finding EquivalenceClasses for a given set of rels

  14. Fix mark-and-restore-skipping test case to not be a self-join.

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