configdb: targeted class-relation lookups instead of full closures - #737
Open
AlexGodbehere wants to merge 1 commit into
Open
configdb: targeted class-relation lookups instead of full closures#737AlexGodbehere wants to merge 1 commit into
AlexGodbehere wants to merge 1 commit into
Conversation
`all_subclass` and `all_membership` are non-materialised views over a recursive CTE. Postgres cannot push `where class = $1` into a recursive CTE, and the `all_class` seed reads `select class from membership`, so every class-relation lookup scanned the whole `membership` table and hashed the whole `object` table, however small the class. ConfigDB's notify layer re-runs one of these per watcher per class change, so a single change fired a burst of O(total objects) queries. That is the query that was saturating PostgreSQL on fpd-ago. Schema v14 adds functions that walk the class graph from one starting point: `class_subclasses` and `class_members` walk down, and `class_has_subclass` and `class_has_member` walk up from the object so an existence test never materialises a class's membership. `model.js` uses them for `_class_lookup`, `_class_has`, `class_has`, `config_class_list`, `config_search` and the redundant-superclass delete in `class_add_subclass`. The views are untouched. Also adds the indexes the schema was missing: `subclass(id)` and `membership(id)` for lookups by object, and `config(object)`, `object(class)` and `object(owner)`, without which deleting one object scanned `config` once and `object` twice. Measured on PostgreSQL 16.14 with the real schema, best of three: 1,000,337 objects before after is object a member? 186.64 ms 0.45 ms subclasses of a class 360.92 ms 0.54 ms members of a leaf class 199.61 ms 11.97 ms delete one object 0.112 s 0.015 s Existence tests and subclass listings are now flat in object count: 0.42, 0.46, 0.46, 0.45 ms at 1,647 / 10,337 / 100,337 / 1,000,337 objects. Answers are unchanged. Verified by differential testing against the views: every object in the database for the lookups, every real relation pair plus random pairs for the existence tests, on both a layered tree and a random DAG. Zero mismatches. Repeated at the application level through `Model` itself: 50,708 comparisons, zero mismatches. This does not address the separate problem that acs-auth expands a class-scoped grant into one ACL entry per object. That needs a targeted permission-check API and is written up separately. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MtATkkjtiFF8L6z98NFD8A
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Review this first
The
is_class()guard inclass_subclassesandclass_has_subclass.all_subclassonly contains the reflexive pair(x, x)whenxis inall_class, so the seed of both walks has to reproduce that condition exactly.class_membersdeliberately has no guard, on the reasoning that a non-class has no members so the join drops the seed. Confirm that holds.This is a schema migration. It adds functions and indexes and does not alter or drop existing objects. The views are untouched and still available.
What this does
Replaces full class-closure scans with graph walks from a single starting point, and adds the indexes the schema was missing.
Why
all_subclassandall_membershipare non-materialised views over a recursive CTE (sql/v8.sql). Two consequences:where k.class = $1is applied after the whole closure is built.all_class, includesselect class from membership, a sequential scan of the entire table.So looking up one leaf class with 333 members reads and hashes every row in
membershipand every row inobject. At 100,000 objects:CDBNotify.class_watchre-runs one of these per watcher on every class change, andacs-authis several of those watchers. These are the two queries seen running repeatedly on fpd-ago while PostgreSQL was saturated:Changes
Schema v14 adds four functions:
class_subclasses(class)class_members(class)class_has_subclass(class, obj)class_has_member(class, obj)Walking up means an existence test never materialises a class's membership.
model.jsuses them in_class_lookup,_class_has,class_has,config_class_list,config_search, and the redundant-superclass delete inclass_add_subclass.Indexes added.
subclassandmembershiphad onlyunique(class, id), so any lookup by object was a sequential scan.confighad onlyunique(app, object), which cannot answer a lookup by object.object.classandobject.ownerare self-references with no index at all.Numbers
PostgreSQL 16.14, real ConfigDB schema, best of three, warm cache. Class tree of 1 root, 20 mid, 300 leaf.
Existence tests are flat in object count: 0.42 / 0.46 / 0.46 / 0.45 ms at 1,647 / 10,337 / 100,337 / 1,000,337 objects.
Listing the members of a class that genuinely holds a million objects is still about a second. That is the cost of returning a million uuids, not a query problem.
Testing
acs-configdbhas no test suite, so this is differential testing against the views on a live PostgreSQL running the real migration chain, v6 through v14.config_class_list, theclass_add_subclassdeleteall_membershiprowsModelmigrate.sqlruns clean from an empty server to v14, and applies v14 on top of an existing v13 database.There is no regression net after this lands, because the service has no test suite.
What this does not fix
acs-authexpands a class-scoped grant into one ACL entry per object and returns the whole list fromGET /v2/acl/:principal. On fpd-ago that was 8,897 entries from 149 stored ACEs over 1,310 datasets.That grows linearly with object count. At 1M datasets it is roughly 6.8M entries and 690 MB of JSON per principal, rebuilt and resent whole on every relevant change, with no delta protocol. There is no targeted "may P do X to O" endpoint.
That needs an API change, not an index, and is written up separately.
🤖 Generated with Claude Code
https://claude.ai/code/session_01MtATkkjtiFF8L6z98NFD8A