perf(adapter): rewrite permissions condition as EXISTS instead of IN subquery#877
perf(adapter): rewrite permissions condition as EXISTS instead of IN subquery#877premtsd-code wants to merge 2 commits into
Conversation
…subquery The previous IN (SELECT ... FROM <coll>_perms) pattern forces MySQL to materialize the entire matching permission set per query. Rewriting as correlated EXISTS lets the planner short-circuit at the first matching _perms row per outer document, which directly reduces innodb_rows_read per SELECT on authenticated reads. Also adds an early '1 = 0' return for empty roles to avoid emitting the invalid 'IN ()' SQL fragment for anonymous users. The method signature is unchanged so all callers (find, count, sum) and adapter subclasses (MariaDB, Postgres, MySQL) inherit the fix without modification.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR refactors SQL permissions condition generation in ChangesPermission Condition Query Optimization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Database/Adapter/SQL.php (1)
1944-1955: Validate/maintain a composite index for the EXISTS path.To consistently realize the expected perf gain, ensure
<collection>_permslookup is indexed on the correlated/filter columns (typically leading with_document, plus_tenantfor shared tables, then_type,_permissionas appropriate).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Database/Adapter/SQL.php` around lines 1944 - 1955, The EXISTS subquery in the SQL generator uses {$permsTable} and filters on perms._document, perms._permission, perms._type and optionally tenant via getTenantQuery; to guarantee the expected performance you must ensure the corresponding <collection>_perms table has a composite index with leading column perms._document, then perms._tenant (if getTenantQuery can apply shared-tenant filtering), followed by perms._type and perms._permission. Add or document a migration/schema change that creates/validates this composite index for the table referenced by $permsTable so the EXISTS(...) path (the correlated condition between {$outerAlias}._uid and perms._document) is covered by the index.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/Database/Adapter/SQL.php`:
- Around line 1944-1955: The EXISTS subquery in the SQL generator uses
{$permsTable} and filters on perms._document, perms._permission, perms._type and
optionally tenant via getTenantQuery; to guarantee the expected performance you
must ensure the corresponding <collection>_perms table has a composite index
with leading column perms._document, then perms._tenant (if getTenantQuery can
apply shared-tenant filtering), followed by perms._type and perms._permission.
Add or document a migration/schema change that creates/validates this composite
index for the table referenced by $permsTable so the EXISTS(...) path (the
correlated condition between {$outerAlias}._uid and perms._document) is covered
by the index.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 051fc0cf-2e18-43b7-9510-eed73a13350c
📒 Files selected for processing (1)
src/Database/Adapter/SQL.php
Greptile SummaryThis PR rewrites
Confidence Score: 5/5Safe to merge — the EXISTS rewrite is logically equivalent to the replaced IN subquery and the only other change is a correctness fix for an edge case that previously produced invalid SQL. The change is narrow (one method, one file), the EXISTS and IN forms produce identical result sets, the inner alias collision concern raised in a prior review has been addressed with the No files require special attention. Important Files Changed
Reviews (2): Last reviewed commit: "fix(adapter): use sentinel alias '_perms..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…llision
Using the unquoted string 'perms' as the inner alias would collide if a
caller ever passed 'perms' as the outer alias. Switching to a leading-
underscore sentinel ('_perms_inner') makes the rewrite robust to any
future caller-provided alias, since outer aliases come from Query::DEFAULT_ALIAS
('main') or explicit user input, neither of which uses the underscore prefix
convention.
1 participant
Summary
Rewrites
getSQLPermissionsConditionfromIN (SELECT ... FROM <coll>_perms)to a correlatedEXISTSclause. This is appended to every authenticatedfind/count/sum, so the per-query cost reduction compounds across the entire platform.The previous
IN (subquery)pattern forces the planner to either materialize the inner permission set or unnest into a semi-join — strategies that scan a non-trivial chunk of_permsper outer query. CorrelatedEXISTSlets the planner short-circuit at the first matching_permsrow per outer document, using the existing_documentindex for single-row lookups.Also adds an early
1 = 0return for empty$roles, so anonymous users no longer produce the invalid_permission IN ()SQL fragment.Why
Production telemetry on Appwrite Cloud (fra region, project DBs) shows ~600 rows scanned per
SELECTon authenticated reads, sustained over a 7-day window. For a typicalLIMIT 25query that's ~24× over-scanning, attributable to the permissions subquery materializing more than necessary.EXISTSdirectly targets this: short-circuit at first match means roughly 1–5_permsrows scanned per outer document instead of the full filtered subset. Predicted impact on the rows-read-per-SELECT ratio: drop from ~600 → ~50–150 (5–10×), translating to ~15–30% MySQL CPU reduction on read-heavy workloads.Scope
SQL.phponly —MariaDB.php,Postgres.php,MySQL.phpinherit the method unchangedfind,count,sumgetTenantQueryalready accepts an alias, so tenant scoping carries through to thepermscorrelation aliasTest plan
composer test)find()withdocumentSecurity: truereturns identical row sets before/aftersum(rate(mysql_global_status_innodb_row_ops_total{operation="read"})) / sum(rate(mysql_global_status_commands_total{command="select"}))drops from baseline (~600) to <200Summary by CodeRabbit
Bug Fixes
Refactor