perf(rls): wrap current_setting() in the generated tenant policy for per-statement evaluation#1468
perf(rls): wrap current_setting() in the generated tenant policy for per-statement evaluation#1468dmitrymaranik wants to merge 4 commits into
Conversation
…per-statement eval
…per-statement eval
…per-statement eval
…per-statement eval
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR modifies SQL generation in TableRLSManager and TraitRLSManager so that ChangesRLS Policy SQL Generation
Estimated code review effort: 1 (Trivial) | ~5 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1468 +/- ##
=========================================
Coverage 86.37% 86.37%
Complexity 1200 1200
=========================================
Files 186 186
Lines 3524 3524
=========================================
Hits 3044 3044
Misses 480 480 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Hi, thanks for the PR. Can you provide a little more context about the mechanics of the cache involved? |
|
Hi @stancl, thanks for taking a look! The "cache" here is Postgres's InitPlan, not an application-level cache.
Wrapping it as It stays semantically equivalent: the GUC is set once (via Happy to add a short benchmark or an inline comment in the PolicyManager if that'd be useful. It came up via pgrls flagging the generated policies (PERF001). |
|
So just to confirm my understanding — this is specifically at query time, the RLS policy itself would be evaluated for each row during a query (that may be trying to select a row from a large table). If we make it a subquery, the result of that subquery will be cached (for the duration of the query?) and therefore only run once. |
|
Exactly right. At query time the predicate is applied to each candidate row the scan touches, so a bare And yes on the scope — it's cached per statement execution: each new query re-runs the InitPlan once, then caches it for that query's rows. Nothing persists across queries, so there's no staleness — a fresh query always reads the current GUC. Net effect: the tenant check goes from O(rows) Happy to drop a one-line comment above the generated policy explaining the |
archtechx/tenancy's RLS policy generator emitscurrent_setting('<tenant setting>')unwrapped in the policy predicate. Postgres re-evaluates a barecurrent_setting(...)once per row the policy scans; wrapping it in a scalar subquery —(select current_setting(...))— makes it a per-statement InitPlan the planner evaluates once and caches. It's predicate-equivalent (tenant isolation unchanged) and the documented Postgres RLS performance pattern; the benefit grows with table size.This wraps the generated call and keeps the test assertions in sync (
TraitRLSManager.php,TableRLSManager.php,TraitManagerTest.php,TableManagerTest.php). I couldn't run the PHP / Laravel test suite locally — if CI surfaces another assertion on the generated policy SQL, point me at it and I'll update it.Spotted with pgrls (an open-source Postgres RLS analyzer). Happy to adjust.
Summary by CodeRabbit