Horton

Re: Removing unneeded self joins

Andrei Lepikhov <lepihov@gmail.com>

View thread
From: Andrei Lepikhov <lepihov@gmail.com>
To: jian he <jian.universality@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>
Cc: Robert Haas <robertmhaas@gmail.com>, Andrei Lepikhov <a.lepikhov@postgrespro.ru>, Alexander Korotkov <aekorotkov@gmail.com>, Richard Guo <guofenglinux@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>
Date: 2024-07-09T06:06:10Z
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.

Attachments

On 7/2/24 07:25, jian he wrote:
> to make sure it's correct, I have added a lot of tests,
> Some of this may be contrived, maybe some of the tests are redundant.
Thanks for your job!
I passed through the patches and have some notes:
1. Patch 0001 has not been applied anymore since the previous week's 
changes in the core. Also, there is one place with trailing whitespace.

Looking into the 0002 and 0003 patches, I think they 1) should be merged 
and 2) It makes sense to use the already existing pull_varnos_of_level 
routine instead of a new walker. See the patches in the attachment as a 
sketch.
Also, I'm not sure about the tests. It looks like we have a lot of new 
tests.

However, the main issue mentioned above is the correctness of relid 
replacement in planner structures.
We have the machinery to check and replace relids in a Query. But 
PlannerInfo is a bin for different stuff that the optimisation needs to 
convert the parse tree to the corresponding cloud of paths.
A good demo of the problem is the introduction of the JoinDomain structure:
It contains a relids field and has no tests for that. We haven't known 
for a long time about the issue of SJE not replacing the relid in this 
structure.
The approach with 'Relation Alias' mentioned by Alexander raises many 
hard questions about accessing simple_rel_array directly or, much worse, 
about checking the scope of some clause where we didn't touch 
RelOptInfo, just compare two relids fields.
The safest decision would be to restart query planning over parse tree 
with removed self-joins, but now planner code isn't ready for that yet.
But maybe we should put this problem on the shoulders of a developer and 
made  something like with nodes: perl script which will generate walker 
switch over PlannerInfo structure?

-- 
regards, Andrei Lepikhov