Fixes #23130: Ensure ltree cascade triggers can be restored from a pg_dump

[jnovinger]

Closes: #23130

The bug

• Cascade triggers gated on WHEN (OLD.path IS DISTINCT FROM NEW.path).
• IS DISTINCT FROM is grammar, not a schema-qualifiable operator: it expands to the operand type's =, resolved from search_path at CREATE TRIGGER time.
• ltree installs into public, so pg_restore (empty search_path) cannot resolve ltree = ltree and the CREATE TRIGGER fails.
• psql does not stop on error by default, so restoring a v4.7.0 dump reported success while omitting all 11 cascade triggers. Descendants then went stale on the next rename or reparent.

The fix

• Compare the paths as text: resolves pg_catalog.text =, always available. sort_path is already text.
• Migrations dcim.0251, tenancy.0027, wireless.0024 reinstall the triggers on existing databases, which carry either the old definition (upgraded in place) or none at all (restored from a dump).
• InstallLtreeTriggers now drops before creating, so it converges from either state instead of failing with 42710. Also unbreaks re-running it for plugins, which the mptt_to_ltree docstring points at.
• Reversing a reinstall is a no-op: the parent reverse drops the functions too, leaving the table with no path maintenance.
• Replication docs now pass -v ON_ERROR_STOP=1. That omission is what made this silent, and it changes behavior: a dump which appeared to restore fine will now abort on its first error.

Stale data

• Rows already stale stay stale. A data migration would rewrite every hierarchical table under a row-exclusive lock during an unattended upgrade, and only some databases are affected.
• Release note carries detection queries for path and sort_path instead. They take no locks, so they can be run against a replica.
• rebuild_ltree_paths does the repair: populate_paths_sql() returns a string for migrations, and rebuild_sort_paths() covers half the problem. It wraps that helper and does nothing else.

The commits

• f6c4d69e0 and ec75e72c7 close ltree cascade triggers fail to restore from a pg_dump (operator does not exist: ltree = ltree) #23130 on their own.
• eb57625a9 is the management command, which is a debatable addition in a patch release. Drop that one commit and only the release note's pointer to it goes with it.

On ::text rather than qualifying the operator

• ltree's text I/O is byte-preserving (memcpy in and out, ltree_eq is a memcmp), so two values are equal iff their text renderings are.
• PostgreSQL guarantees this nowhere. On the same bug upstream (#17134) a committer suggested OPERATOR("schema"."=") or a function-body check.
• My view is ::text fits better: it hardcodes no schema, so it survives ltree living outside public. Open to the body-guard alternative if you'd rather not lean on that.

Reproduced against the published v4.7 demo dump, which needs regenerating once this ships.

[jnovinger]

@claude /review Only report on issues of potential concern; we don't need commentary on anything that doesn't require further consideration.

[github-actions]

Claude finished @jnovinger's task in 5m 44s —— View job

---

Review: Fixes #23130 — restore-safe ltree cascade triggers

• Gather PR diff and context
• Review utilities/ltree.py trigger changes
• Review migrations (dcim/tenancy/wireless)
• Review rebuild_ltree_paths management command
• Review docs + tests
• Post findings

The core fix is sound. I verified the table coverage against 0242_ltree_paths / 0025_ltree_paths / 0021_ltree_paths (11 tables, all reinstalled), the migration leaf numbers are conflict-free, InstallDenormalizationTrigger's existing callers all watch integer FK columns (so the new warning is accurate and there is no second instance of the bug), and path::text equality is bit-identical to ltree_eq for a deterministic default collation. Findings below are ordered by severity.

---

1. --dry-run reports OK for tables whose only damage is a stale sort_path

netbox/utilities/management/commands/rebuild_ltree_paths.py:63-71 — count_inconsistent() only compares path. But the release notes this PR adds explicitly say a rename cascades sort_path and not path, and give a separate sort_path detection query for exactly that reason.

Failure: on a restored v4.7.0 database with the cascade trigger missing, rename a parent Region. The BEFORE trigger updates the parent's own sort_path; descendants keep the old one. path is untouched, so rebuild_ltree_paths --dry-run prints dcim.region: OK and the operator concludes the table is clean — while child ordering is wrong and the release note's own second query would report the rows. Since the release note points operators at this command as the repair tool, the dry run needs to cover both columns (OR in the sort_path predicate from the release note, gated on model._has_sort_path()).

Fix this →

2. The count query runs on every rebuild and is then discarded

rebuild_ltree_paths.py:79 computes inconsistent unconditionally, but it is only read inside the --dry-run branch. In rebuild mode that's a wasted self-join per table, and — more importantly — the obvious use for it is skipped: a plain rebuild_ltree_paths rewrites every row of every hierarchical table, including dcim_inventoryitem, even when the count says the table is clean. The command's own comment at line 91 flags that this locks the whole table. Either gate the rebuild on inconsistent (once it also covers sort_path, per #1) or move the count into the dry-run branch.

3. populate_paths_sql() hardcodes name, but InstallLtreeTriggers accepts any name_column

get_models() deliberately supports plugin models when named explicitly (rebuild_ltree_paths.py:26-33), and populate_paths_sql() builds sort_path from a literal name column (utilities/mptt_to_ltree.py:97,102). A plugin that installed its triggers with e.g. name_column='label' will either error with column "name" does not exist or, if the model happens to have an unrelated name field, silently rewrite every sort_path from the wrong column — corrupting data the command was invoked to repair. Same for count_inconsistent(). Worth either threading the name column through, or rejecting plugin models whose trigger definition doesn't watch name (readable from pg_get_triggerdef).

4. New command is not in docs/administration/management-commands.md

Every other operator-facing command (rebuild_prefixes, renaturalize, calculate_cached_counts, …) has a section there. rebuild_ltree_paths is only mentioned in the release note, which will scroll out of relevance.

5. dcim/0251 docstring understates the lock

"it does not incur the table-wide lock that 0242's path backfill did"

DROP TRIGGER / CREATE TRIGGER take ACCESS EXCLUSIVE on the table — briefly held, but a stronger lock than the backfill's ROW EXCLUSIVE, and it blocks readers too. On a busy dcim_inventoryitem it queues behind any long-running query and stalls everything behind it for that duration. The claim as written reads as "safe to run under load"; suggest rewording to "acquires ACCESS EXCLUSIVE only for the DDL itself, with no table scan".

6. dcim/0251 docstring: reversing 0242 drops the triggers, it doesn't recreate them

"the triggers it replaces belong to 0242_ltree_paths, which recreates them (from the corrected template) when reversed in turn"

InstallLtreeTriggers.database_backwards (utilities/ltree.py:374-379) only issues DROP TRIGGER / DROP FUNCTION. The ReinstallLtreeTriggers docstring's phrasing ("recreated by reversing back to the migration which installed them") is defensible if read as un-applying down to but not including 0242, but the dcim/0251 wording is not. The conclusion — no-op reverse is correct — still holds; only the justification is wrong.

7. Minor: the trigger-definition assertion is coupled to PostgreSQL's deparse output

test_ltree.py asserts the literal '(old.path)::text IS DISTINCT FROM (new.path)::text' in pg_get_triggerdef(). Parenthesization and casing there are an implementation detail of ruleutils.c; NetBox supports PG 15+ and this will need touching if a future major changes the rendering. Asserting '::text' is present and the un-cast form is absent would be equally strong and version-agnostic.

8. Convention: release-note section added in the PR

AGENTS.md asks that PRs not add release-note entries (they're generated in aggregate to avoid merge conflicts). The v4.7.1 warning block here is substantive operator guidance that clearly belongs somewhere, so this is a maintainer call rather than a defect — flagging only because the bug-fix bullet at minimum will conflict at release time.

---

Not a concern, checked: the ::text cast resolves via pg_cast by type OID so it needs no search_path; count_inconsistent()'s unqualified ::ltree is fine because the ORM already emits unqualified ltree operators at runtime; _corrupt() in the new test correctly bypasses the triggers (AFTER UPDATE OF parent_id, name); the SET LOCAL search_path in RestoreUnderRestrictedSearchPathTests is contained by the class-level atomic; the 11-table set derived from LtreeModel.__subclasses__() matches the migrations exactly.

I was not able to run ruff check or the test suite in this environment (command approval / no database), so the findings above are from reading only.
• branch 23130-fix-ltree-trigger-restore

[jnovinger]

@claude /review Only report on issues of potential concern; we don't need commentary on anything that doesn't require further consideration.

Thanks, that was a useful pass. Changes are in ae3dbb285 (fix) and 6259b5b31 (command), kept in separate commits so the command still drops cleanly.

1. --dry-run reports OK for tables whose only damage is a stale sort_path

Fixed. count_inconsistent() now returns both counts and the dry run reports them separately. You were right that this was the worst of the eight: the release note points operators at this command, so reporting OK on a rename-damaged table would have sent them away satisfied. Two tests added, one asserting the dry run does not print OK when only sort_path is stale, one asserting the repair fixes it.

2. The count query runs on every rebuild and is then discarded

Fixed, and I took the stronger option: a rebuild now skips tables with nothing to repair rather than rewriting every row of every table. --force rebuilds regardless, because a zero count does not prove a tree is sound. A node whose ancestor moved is reported; its own descendants agree with it and are not.

3. populate_paths_sql() hardcodes name, but InstallLtreeTriggers accepts any name_column

Fixed by refusing rather than threading it through. A model which maintains sort_path without a name field now raises CommandError. Reading the column back out of pg_get_triggerdef would work, but a plugin in that position is better served by its own repair path than by core guessing.

4. New command is not in docs/administration/management-commands.md

Added, between rebuild_config_context_cache and rebuild_prefixes.

5. dcim/0251 docstring understates the lock

Corrected. The docstring now says ACCESS EXCLUSIVE for the DDL with no table scan, and notes it is stronger than the backfill's ROW EXCLUSIVE and blocks readers.

6. dcim/0251 docstring: reversing 0242 drops the triggers, it doesn't recreate them

Corrected. The justification was wrong; the conclusion was not.

7. Minor: the trigger-definition assertion is coupled to PostgreSQL's deparse output

Fixed as suggested: asserts ::text IS DISTINCT FROM is present and the un-cast form is absent.

8. Convention: release-note section added in the PR

I dropped the bug-fix bullet, which is the part that conflicts at release time. I'm keeping the operator warning deliberately: a database restored from a v4.7.0 dump may already be silently corrupt, and that is upgrade-time guidance a changelog bullet would not carry. Happy to move it into replicating-netbox.md instead if you'd rather the release notes stay fully generated.

CI is green on 3.12, 3.13 and 3.14. Locally, 79 tests across test_ltree and test_management_commands pass, and zensical build reports no issues.

[github-actions]

Claude finished @jnovinger's task in 3m 48s —— View job

---

Review: Fixes #23130 — restore-safe ltree cascade triggers (round 2)

• Fetch updated diff (ae3dbb285, 6259b5b31)
• Verify each of the 8 prior findings is addressed
• Review the new/changed code for fresh issues
• Post findings

Prior round: #1, #4, #5, #6, #7 are resolved as described — I re-read each. #2 is resolved, and the skip logic is sound for the #23130 scenario specifically: the BEFORE trigger has no WHEN clause, so it survives a restore intact, which means every rename/reparent damaged under a missing cascade trigger leaves at least one child disagreeing with its parent and is therefore counted. #3 is only half resolved — see below. #8 is answered at the end.

Two concerns, both in rebuild_ltree_paths.py.

---

1. check_sort_path_column() guards the loud failure, not the silent one

netbox/utilities/management/commands/rebuild_ltree_paths.py:67-82 checks only that a Django field named name exists. That is the case which was already loud — populate_paths_sql() would have aborted the transaction with column "name" does not exist, repairing nothing. The case the original finding was actually about is the other one, and it still passes the guard:

A plugin model with both a label field (triggers installed via InstallLtreeTriggers(table, name_column='label')) and an unrelated name field satisfies get_field('name'), so the command proceeds and populate_paths_sql() rewrites every sort_path from name — silently replacing correct data with wrong data in the table the operator invoked the command to repair. count_inconsistent() at line 104 has the same assumption, so the dry run will also report the (correct) table as damaged, actively steering the operator into the rewrite.

If threading the column through isn't wanted, the guard needs to assert the trigger's column rather than the model's. pg_get_triggerdef() for {table}_ltree_compute_path renders the watched columns (BEFORE INSERT OR UPDATE OF parent_id, label ON ...), so refusing when that list doesn't contain name closes both halves with the same amount of code.

Two smaller things in the same function:

• except Exception (line 78) should be except FieldDoesNotExist — as written it converts any _meta problem into a "has no name field" message.
• get_field('name') matches the Django field name, not the column. name = CharField(db_column='label') passes and then fails in SQL.
• The check runs inside the per-model loop (line 117), so on a multi-model invocation the CommandError fires after earlier tables have already been rebuilt. Validating all models before the loop would make the refusal a clean no-op.

Fix this →

2. Detection excludes root rows, and detection now gates the repair

Both queries in count_inconsistent() (lines 94 and 103) are JOIN {table} p ON c.parent_id = p.id — an inner join, so rows with parent_id IS NULL are never examined. Under the old unconditional rebuild that was harmless: roots got rewritten along with everything else. Now that damage decides whether the table is touched at all (line 139), an unexamined row is an unrepaired row.

Failure: a root with no children whose path was hand-edited, or a table which is entirely flat (all roots) with damaged paths. --dry-run prints OK, a plain run prints OK, skipping, and Rebuilt 0 table(s) — the command reports the table clean while it isn't. --force is the escape hatch, but the operator has no reason to reach for it after being told OK.

Narrow in the #23130 restore scenario (roots aren't damaged there, per the note above), but the command documents itself as the general repair tool for "a bulk COPY, a direct UPDATE", where root damage is reachable. A UNION ALL arm over WHERE parent_id IS NULL AND path <> lpad(id::text, 19, '0')::ltree (and sort_path <> name) covers it.

Fix this →

---

Minor

• docs/administration/management-commands.md — the model list omits inventory item templates. The command's default set is every concrete LtreeModel subclass (11 tables, including dcim.inventoryitemtemplate), and the release note lists all 11; the command doc lists 10.
• Same file, the warning block: an UPDATE takes ROW EXCLUSIVE on the table, which does not block other writers — what blocks them here is the per-row tuple locks, which this statement happens to take on every row. The conclusion ("blocks concurrent writes, use a maintenance window") is right; the named mechanism isn't. Inherited from the populate_paths_sql() docstring, so it's two places if you want it consistent. Flagging only because 0251's docstring just got tightened on exactly this axis.

On #8 (release note)

Keep it where it is. What's left after dropping the bullet is an upgrade-notes admonition, not a changelog entry, and it's the same shape as the "PostgreSQL 15 or Later Required" block directly below it in the same file — which is the precedent for this content living in the release notes. replicating-netbox.md is the wrong home: an operator restoring a dump today reads it, but the audience that needs this is the one that already restored a v4.7.0 dump weeks ago and will only find out at upgrade time. The merge-conflict surface the convention exists to protect is gone now that the bullet is.

---

I could not run ruff check or the test suite here (no database, and gh/shell approvals were limited), so the above is from reading the branch at 6259b5b31.
• branch 23130-fix-ltree-trigger-restore

[jnovinger]

• Force-pushed, so the SHAs in my previous comment are gone. More usefully, so is most of what that comment described.

• rebuild_ltree_paths is now a wrapper around populate_paths_sql() and nothing else: 66 lines, down from 152. Removed the inconsistency counting, --dry-run, --force, the skip-if-clean logic, and the sort_path column guard.

  That was a scope decision rather than a review fix. The command exists because populate_paths_sql() returns a SQL string an operator can't call, and everything I'd added past that wrapper was generating problems of its own: hand-written detection SQL which re-derived the path label width and sort_path separator that mptt_to_ltree already owns, assembled with f-strings, and inner-joined on parent_id so root rows went unexamined. Once detection gated the rebuild, an unexamined row became an unrepaired one. Detection now lives only as the release-note queries, which take no locks and need nothing installed, so they can be run against a replica.

  Plugin models are excluded outright instead of guarded. populate_paths_sql() reads the name column by name while InstallLtreeTriggers accepts any column, and my guard checked whether the model had a name field, which passes for a plugin maintaining sort_path from something else that also happens to have one. Excluding them removes the hazard rather than trying to detect it.

• The 0251 docstring claims about locking and reversal were both wrong and are fixed in ec75e72c7: reinstalling a trigger takes ACCESS EXCLUSIVE, not the ROW EXCLUSIVE that 0242's backfill held, and reversing 0242 drops these triggers rather than recreating them. The no-op reverse is still right, just for a different reason than I'd written.

• f6c4d69e0 and ec75e72c7 close ltree cascade triggers fail to restore from a pg_dump (operator does not exist: ltree = ltree) #23130 on their own. eb57625a9 is the command, still a single commit, still droppable if a new management command isn't wanted in a patch release.