Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Identify areas of the code that could be improved with Common Table Expressions (aka WITH clauses) #10962

Open
GuySartorelli opened this issue Sep 26, 2023 · 0 comments

Comments

@GuySartorelli
Copy link
Member

GuySartorelli commented Sep 26, 2023

#10902 adds a new abstraction API for common table expressions (CTEs).

CTEs are a powerful way to create recursive queries (e.g. to find all ancestors - parents of parents - for a given record) and to optimise queries which are, for example, adding a join to a sub-query (e.g. SELECT x FROM y INNER JOIN (SELECT a FROM b);)

Notes

  • Important: This might result in a lot of new cards created. Do we want to create an epic for it? Alternatively, do we want to put the identified areas in a spreadsheet or a comment or something first, and review them, and potentially only turn some of those into cards after a review?
  • FileLink and SiteTreeLink are potentially good candidates for non-recursive CTE queries to replace the subselect in the JOIN clause.
  • I've done some rough benchmarking on Hierarchy::getAncestors() and for a fairly standard scenario with only 4 levels of hierarchy there's only a 3% improvement using CTEs.... so it's not going to be worth it unless methods are used many times in a given request, or are expected to have deep nesting, or are perhaps recursive for some other reason.
  • Various methods which could benefit from recursive queries were identified in the description of Add ORM abstraction for "WITH" clauses #10902
  • Replacing subqueries with CTEs doesn't affect performance, it just makes the resulting query more readable. This is nice for debugging, but since we have to support databases that don't have CTE support I don't think it's worth replacing those at this stage.

Acceptance criteria

  • Areas of the codebase of core and supported modules are identified which currently use recursive methods which could be swapped out for recursive CTE queries
  • New cards are created to update the code to use CTE queries where appropriate
    • Cards explicitly call out the need to check DB::get_conn()->supportsCteQueries() and only apply the new logic if the result is true
    • Cards explicitly call out when the new CTE behaviour needs to be opt-in to avoid backwards-compatibility breaking changes
    • Cards explicitly call out that the new CTE implementation must represent either a significant performance improvement, a significant code quality improvement, or both - and must not deteriorate performance in any foreseeable scenario.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants