#21418 - Replace MPTT wtih PostgreSQL Ltree#22296
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
@claude /review Only report on issues of potential concern; we don't need commentary on anything that doesn't require further consideration. |
|
Claude finished @arthanson's task in 12m 0s —— View job PR Review: #21418 — Replace MPTT with PostgreSQL Ltree
The overall implementation is well-engineered — the trigger design, advisory-lock concurrency model, and MPTT-compatible API surface are all sound. The test suite is thorough. Below are issues that warrant attention. 1.
|
|
Claide review comments addressed. |
jeremystretch
left a comment
There was a problem hiding this comment.
Amazing work! I haven't dug into the triggers or reviewed any of the new tests yet, but wanted to push through some general feedback/questions. A couple general thoughts:
- I don't know how closely we care to mimic the MPTT API. IMO we should provide only methods/properties that have a solid use case. (We can always extend these later, but will need to maintain indefinitely whatever we initially expose.)
- We'll need to do a ton of testing at scale to catch performance issues, especially concerning the initial migrations.
- We should probably break up the
netbox.models.ltreemodule as it's quite large. - This is the most optimistic I've ever felt about finally ditching MPTT. 🙂
|
@jeremystretch removed un-used MPTT methods, cleaned up comments and refactored. Please note two comments on your suggestions above. |
|
@jeremystretch other review comments addressed. |
jeremystretch
left a comment
There was a problem hiding this comment.
This all looks clean AFAICT. However, Claude did raise a performance concern during review, which I'll share below. The proposed modification to _LOCK_TREE_ROOTS_SQL seems reasonable, but I wanted to get your take @arthanson.
Advisory-lock pool exhaustion on large single-transaction bulk inserts (worth confirming)
The BEFORE trigger takes a pg_advisory_xact_lock keyed per-tree, held until commit. For new root nodes, dest_root is the row's own label, so each new root is a distinct key. A single atomic transaction that bulk-creates many roots (e.g. a large CSV import of Regions/TenantGroups, or a script inside transaction.atomic()) accumulates one advisory lock per root, all held simultaneously, drawing from the shared lock table (max_locks_per_transaction × (max_connections + …)).
MPTT took a single per-model lock; this is strictly more locks for the wide-root case, and at sufficient volume can fail with out of shared memory / You might need to increase max_locks_per_transaction. The per-tree concurrency win is real and worth it — but has this been load-tested with a large single-transaction import of many roots? If it's a known limit, a note in the docs/release notes would help operators.
Primary fix: don't lock on root inserts
The exhaustion comes from one fact: a brand-new root node takes an advisory lock keyed on its own label, held until commit — but that lock can never contend with
anything. A root being INSERTed with parent_id IS NULL starts its own singleton tree that no concurrent transaction can see or reference yet (MVCC — the uncommitted
row isn't visible, and nobody else has its PK). So:
- It can't be reparented concurrently (doesn't exist for other txns).
- Its subtree is empty.
- No other transaction's AFTER cascade can target it (it's in nobody's tree).
The lock the trigger's own comment justifies — "serialize an insert against a concurrent reparent of one of its ancestors" — is meaningless for a node that has no
ancestor. So the lock on a new-root key is pure overhead and the entire pool-consumption vector, since each root is a distinct key.
The change is a guard at the top of _LOCK_TREE_ROOTS_SQL:
-- A brand-new root (INSERT, no parent) starts its own singleton tree that no
-- concurrent transaction can yet see or reference, so there is nothing to
-- serialize against. Skipping the lock here is what keeps a bulk import of many
-- top-level objects from accumulating one xact-lock per root and exhausting the
-- shared lock table (max_locks_per_transaction).
IF TG_OP = 'INSERT' AND NEW.parent_id IS NULL THEN
-- no lock required
ELSE
-- ... existing dest_root / old_root / pg_advisory_xact_lock logic ...
END IF;This is safe and narrow:
- A reparent-to-root (
UPDATE … parent_id = NULL) still locks — the node exists, has descendants, and the cascade rewrites a real subtree. - A child insert into an existing tree still locks.
- Only the provably race-free case is exempted.
Critically, this kills the exact scenario I flagged: importing N top-level objects (Regions, TenantGroups, or one inventory item per device — InventoryItem trees are
rooted at top-level items, so each is its own root) now takes zero advisory locks instead of N. The dominant bulk pattern becomes lock-free.
Residual case + operator fallback
The primary fix doesn't cover bulk-inserting children into many distinct existing trees in one transaction (e.g. one child under each of 50k existing trees) — that
still accumulates one lock per distinct tree. This is a much rarer pattern, and for it the right answer is operator guidance:
- Document that very large hierarchical bulk writes scale with the number of distinct existing trees touched per transaction, and that
max_locks_per_transactionis
the tuning knob if out of shared memory appears. This is the standard Postgres remedy and a one-line config bump.
I'd put that in the migration/upgrade notes alongside the existing operator warning.
What I'd avoid
- Releasing the lock mid-transaction (session-level
pg_advisory_lock+ early unlock) is not safe: the correctness argument requires holding until commit, or a
concurrent reparent could interleave between the path computation and commit and leave a stale path this txn can no longer fix. Don't go there. - Per-batch commits in bulk import would release locks between batches but breaks NetBox's all-or-nothing atomic import semantics — not worth it.
- Coarse whole-table lock for bulk defeats the per-tree concurrency that's the whole point of the PR.
Verification
Add a regression test that pins the behavior: inside a transaction, create a root and assert no advisory locks are held —
with transaction.atomic():
Region.objects.create(name='Root', slug='root')
with connection.cursor() as cur:
cur.execute("SELECT count(*) FROM pg_locks WHERE locktype = 'advisory'")
self.assertEqual(cur.fetchone()[0], 0)and a companion asserting a child insert (or reparent-to-root) does hold exactly one, so the optimization can't silently over-broaden later.
|
Updated the lock code. |
Closes: #21418
Replaces django-mptt with PostgreSQL ltree for all hierarchical models in NetBox core. Eliminates the read/write/maintenance costs of MPTT's lft/rght/tree_id/level columns and the table-wide lock MPTT takes on every insert/reparent, in favor of a single GiST-indexed path column maintained entirely by database triggers.
Expected Performance Improvements
The biggest gains are on writes and concurrency, and they scale with tree size - so the speedup is data-dependent rather than a fixed factor. An MPTT insert/move is O(N) in the tree (it renumbers lft/rght across much of the tree, and shifts tree_id across trees on a root insert), whereas ltree only recomputes the affected node and its subtree (≈O(1) for a leaf insert, O(subtree) for a move) and maintains one path column instead of MPTT's four.
For small/shallow hierarchies the two are roughly on par; for large hierarchies a single write touches one row instead of thousands, so it can be an order of magnitude or more faster. Separately, MPTT's per-model lock serialized every write to the model; ltree locks per tree, so writes to different trees (e.g. components on different devices) run concurrently - bulk imports and automation that previously queued on one lock should see throughput improve roughly in proportion to how many independent trees are written in parallel.
Read performance for ancestor/descendant lookups is comparable (both index-backed), so queries are unaffected.
New infrastructure (netbox/netbox/models/ltree.py)
NestedLtreeGroupModelis the ltree-backed base for hierarchical group models (Region, Location, Platform, etc.), carrying two trigger-maintained columns: path, an ltree of ancestor PKs for fast ancestor/descendant queries, and sort_path, a chr(9)-joined chain of ancestor names. sort_path exists because ordering by path sorts siblings by PK (insertion order), whereas ORDER BY sort_path reproduces MPTT's order_insertion_by=('name',) - depth-first tree-flatten with siblings in name order (the TAB separator keeps each subtree grouped under its parent), and its collation tracks name's automatically.Base model changes (netbox/netbox/models/init.py)
Models converted
ModuleBay, InventoryItem, and InventoryItemTemplate use LtreeModel directly (not the nested-group base) since they aren't NestedGroupModel subclasses.
Behavioral changes vs MPTT
ValidationErrorboth inclean()(forms/serializers) and insave()(scripts/bulk), mirroring MPTT's save-timeInvalidMove.ModuleBayopts out (its parent is derived from its installed module).sort_pathimmediately and cascade to descendants. MPTT'sorder_insertion_byleft descendants stale until a manual rebuild.LtreeModel.rebuild_sort_paths()remains for repairing raw writes that bypass the triggers.GraphQL
The MPTT-internal
lft/rght/tree_idcolumns are no longer part of the schema.levelis preserved as a resolved field (nlevel(path) - 1).Removed
MPTTLockedMixinand the per-model MPTT tree locks - i.e. the MPTT-specific entries (region, sitegroup, location, tenantgroup, contactgroup, wirelesslangroup, inventoryitem, inventoryitemtemplate, platform) inADVISORY_LOCK_KEYS; the delay_mptt_updates() / rebuild() wrappers in the bulk import/edit/rename views; and utilities/templatetags/mptt.py.django-pglocks and ADVISORY_LOCK_KEYS themselves are retained — they're still used for available-prefix/IP/VLAN/ASN allocation and job scheduling. django-mptt is retained only for historical migrations and the deprecated NestedGroupModel.
Migrations
Per-app migrations enable the
ltreeextension, add thepath(andsort_pathwhere applicable) columns, backfill existing rows via a recursive CTE, add GiST indexes, and install the triggers. No operator action is required beyond running migrations.And the inventory-item ordering line (separate, in case you place it under the conversion notes):
Test Script
Below is a script that adds nested items to ModuleBays, Inventory Items, etc.. to check screens on MPTT and LTree. So run it once on MPTT version, open all the tabs, then run it on LTree version and in new browser window check same tabs. can do command like:
python netbox/manage.py shell < ./pr_reviews/demo_tree_ordering.py > /tmp/ltree.out 2>&1demo_tree_ordering.py