Report which models need their ltree paths rebuilt

Finding out whether a database was affected meant running two SQL queries by
hand against each of eleven tables. Add a --check mode which does that: it
reports the models whose stored paths disagree with their parent relationships
and exits, modifying nothing and taking no locks, so it can be run on a live
system or against a replica.

It deliberately does not decide what to rebuild. The counts answer whether a
model needs rebuilding, not how many of its objects are wrong: where an object
has moved, the objects beneath it still agree with their own parent and are not
counted, though they are equally stale. Skipping a model on a zero count would
therefore skip real damage, so a rebuild still rewrites every model it is given.

The refusal for unreachable rows now names them. Telling an operator to correct
the parent relationships without saying which ones is not something they can act
on: eleven tables and no indication of where to look. It lists the first ten ids
and marks the list as truncated where there are more.

The SQL detection queries stay in the release notes, since an operator still on
v4.7.0 cannot run a command which does not exist there yet.

docs/administration/management-commands.md

@@ -48,16 +48,53 @@ python3 netbox/manage.py rebuild_config_context_cache [--force]
 
 Recompute the `path` and `sort_path` columns of the hierarchical models (regions, site groups, locations, device roles, platforms, tenant groups, contact groups, wireless LAN groups, module bays, inventory items, and inventory item templates) from their parent relationships. These columns are maintained by PostgreSQL triggers, so this is needed only where a write bypassed them: a bulk `COPY`, a direct `UPDATE`, or a database restored from a NetBox v4.7.0 dump (see [#23130](https://github.com/netbox-community/netbox/issues/23130)).
 
-Pass one or more models as `app_label.ModelName` to limit the rebuild.
+The command has two modes. Both operate on every hierarchical model by default, or on those named as `app_label.ModelName`.
+
+### Reporting
+
+`--check` compares each object's stored `path` and `sort_path` against its parent's and reports which models disagree. It modifies nothing and takes no locks, so it can be run on a live system or against a replica.
+
+```
+python3 netbox/manage.py rebuild_ltree_paths --check
+```
+
+```no-highlight
+dcim.location: 5 path, 5 sort_path row(s) disagree with their parent
+dcim.region: 2 sort_path row(s) disagree with their parent
+...
+
+Needs rebuilding: dcim.location dcim.region
+```
+
+The counts answer whether a model needs rebuilding, not how many of its objects are wrong. Where an object has moved, the objects beneath it still agree with their own parent and are not counted, though they are equally stale. Rebuild the whole model rather than acting on the number.
+
+A model can also be damaged in a way `--check` does not report: an object which no root reaches by following `parent_id` is compared against a parent that is itself unreachable, so it may agree and be counted clean. The rebuild detects that case and refuses (see below).
+
+### Rebuilding
+
+With no `--check`, each named model is rebuilt: every row's `path` and `sort_path` are recomputed from the hierarchy.
 
 ```
 python3 netbox/manage.py rebuild_ltree_paths [app_label.ModelName ...]
 ```
 
-The rebuild derives each object's path by walking down from the roots of the hierarchy, so it can only repair a row which some root reaches by following `parent_id`. Where a table contains a row that no root reaches — one belonging to a cycle, one parented to itself, or one whose parent no longer exists — the command reports the count and stops without modifying that table, because a rebuild would silently skip exactly those rows. Correct the parent relationships and run it again.
+```no-highlight
+dcim.region: rebuilding... done
+Finished.
+```
+
+A rebuild derives each object's path by walking down from the roots, so it can only repair an object which some root reaches. Where a model contains an object no root reaches — one in a cycle, one parented to itself, or one whose parent no longer exists — the command reports how many and stops without modifying that model, because a rebuild would silently skip exactly those objects:
+
+```no-highlight
+CommandError: dcim.region: 5 row(s) cannot be reached from a root by following
+parent_id, so a rebuild would skip them: 1, 2, 3, 4, 5. Correct the parent
+relationships, then re-run.
+```
+
+One of the listed objects is in a cycle, parented to itself, or pointing at an object which no longer exists; the rest are descended from it and are otherwise intact. Correcting the relationship is left to the operator, as only they can say what the hierarchy was meant to be. Each model is checked and rebuilt in its own transaction, so a refusal leaves that model untouched, and models already rebuilt stay rebuilt.
 
 !!! warning
-    A rebuild rewrites every row of each named table in a single statement, locking those rows until it commits. On a large table this blocks concurrent writes for minutes, so run it during a maintenance window. The detection queries in the [v4.7.1 release notes](../release-notes/version-4.7.md) take no locks, and can be used first to find which tables need it.
+    A rebuild rewrites every row of each named model in a single statement, locking those rows until it commits. On a large table this blocks concurrent writes for minutes, so run it during a maintenance window. Use `--check` first to limit the rebuild to the models which need it.
 
 ## rebuild_prefixes
 

docs/release-notes/version-4.7.md

@@ -5,7 +5,13 @@
 !!! warning "Databases Restored From a v4.7.0 Dump"
     The triggers which cascade a hierarchical object's path to its descendants could not be recreated when restoring a `pg_dump` of a v4.7.0 database, because `pg_dump` resets the `search_path` and the triggers' `WHEN` clause depended on it. As `psql` does not stop on error by default, such a restore reported success while leaving the database without those triggers, so renaming or moving a region, site group, location, device role, platform, tenant group, contact group, wireless LAN group, module bay, or inventory item did not update its descendants.
 
-    Upgrading reinstalls the triggers, so all subsequent changes are cascaded correctly. It does **not** repair values which have already gone stale. The query below reports whether a table is affected. Substitute each hierarchical table in turn: `dcim_region`, `dcim_sitegroup`, `dcim_location`, `dcim_devicerole`, `dcim_platform`, `dcim_modulebay`, `dcim_inventoryitem`, `dcim_inventoryitemtemplate`, `tenancy_tenantgroup`, `tenancy_contactgroup`, and `wireless_wirelesslangroup`.
+    Upgrading reinstalls the triggers, so all subsequent changes are cascaded correctly. It does **not** repair values which have already gone stale. After upgrading, `rebuild_ltree_paths --check` reports which models are affected without modifying anything or taking any locks:
+
+    ```no-highlight
+    python netbox/manage.py rebuild_ltree_paths --check
+    ```
+
+    To check before upgrading, the same test can be run as SQL. Substitute each hierarchical table in turn: `dcim_region`, `dcim_sitegroup`, `dcim_location`, `dcim_devicerole`, `dcim_platform`, `dcim_modulebay`, `dcim_inventoryitem`, `dcim_inventoryitemtemplate`, `tenancy_tenantgroup`, `tenancy_contactgroup`, and `wireless_wirelesslangroup`.
 
     ```no-highlight
     SELECT count(*) FROM dcim_region c JOIN dcim_region p ON c.parent_id = p.id

netbox/utilities/management/commands/rebuild_ltree_paths.py

@@ -4,7 +4,11 @@ from django.db import connection, transaction
 
 from netbox.models.ltree import LtreeModel
 from netbox.plugins import PluginConfig
-from utilities.mptt_to_ltree import count_unreachable_rows_sql, populate_paths_sql
+from utilities.mptt_to_ltree import (
+    count_stale_rows_sql,
+    populate_paths_sql,
+    unreachable_rows_sql,
+)
 
 
 class Command(BaseCommand):
@@ -13,11 +17,18 @@ class Command(BaseCommand):
         "from their parent relationships"
     )
 
+    # How many offending ids a refusal names. Enough to start from, short enough to read.
+    REPORTED_IDS = 10
+
     def add_arguments(self, parser):
         parser.add_argument(
             'model', nargs='*',
             help="Limit the rebuild to these models, as app_label.ModelName (default: all)",
         )
+        parser.add_argument(
+            '--check', action='store_true',
+            help="Report which models need rebuilding, without modifying anything",
+        )
 
     def get_models(self, names):
         """
@@ -65,27 +76,70 @@ class Command(BaseCommand):
         transaction as the rebuild it guards. Checking in a separate transaction would
         leave a window in which a concurrent write could strand a row between the two.
         """
-        cursor.execute(count_unreachable_rows_sql(model._meta.db_table))
-        unreachable = cursor.fetchone()[0]
+        cursor.execute(unreachable_rows_sql(model._meta.db_table, self.REPORTED_IDS))
+        unreachable, ids = cursor.fetchone()
 
         if unreachable:
+            listed = ', '.join(str(pk) for pk in ids)
+            if unreachable > len(ids):
+                listed += ', ...'
             raise CommandError(
                 f'{model._meta.label_lower}: {unreachable} row(s) cannot be reached from a '
-                f'root by following parent_id, so a rebuild would skip them. A cycle, a row '
-                f'parented to itself, or a parent_id referencing a missing row will do this. '
+                f'root by following parent_id, so a rebuild would skip them: {listed}. '
                 f'Correct the parent relationships, then re-run.'
             )
 
+    def report_stale(self, model):
+        """
+        Report whether a model's stored paths disagree with its parent relationships.
+
+        Read-only, and takes no locks, so it can be run outside a maintenance window or
+        against a replica. It answers which models need rebuilding, not how many rows are
+        damaged: see `count_stale_rows_sql()` for why the counts understate a deep tree.
+        """
+        with connection.cursor() as cursor:
+            cursor.execute(
+                count_stale_rows_sql(model._meta.db_table, sort_path=model._has_sort_path())
+            )
+            stale_paths, stale_sort_paths = cursor.fetchone()
+
+        if not (stale_paths or stale_sort_paths):
+            self.stdout.write(f'{model._meta.label_lower}: OK')
+            return False
+
+        damage = []
+        if stale_paths:
+            damage.append(f'{stale_paths} path')
+        if stale_sort_paths:
+            damage.append(f'{stale_sort_paths} sort_path')
+        self.stdout.write(self.style.WARNING(
+            f"{model._meta.label_lower}: {', '.join(damage)} row(s) disagree with their parent"
+        ))
+        return True
+
     def handle(self, *args, **options):
+        models = self.get_models(options['model'])
+
+        if options['check']:
+            stale = [model for model in models if self.report_stale(model)]
+            if stale:
+                names = ' '.join(model._meta.label_lower for model in stale)
+                self.stdout.write(f'\nNeeds rebuilding: {names}')
+            else:
+                self.stdout.write(self.style.SUCCESS('Nothing to rebuild.'))
+            return
+
         # Each table is checked and rebuilt in its own transaction. Tables already done
         # stay done if a later one fails or is refused: rebuilding one table cannot leave
         # another inconsistent, and holding every table's row locks until the last one
         # finished would turn several short blocking windows into one long one.
-        for model in self.get_models(options['model']):
-            self.stdout.write(f'{model._meta.label_lower}: rebuilding... ', ending='')
-            self.stdout.flush()
+        for model in models:
             with transaction.atomic(), connection.cursor() as cursor:
+                # Announce the rebuild only once the check has passed, so a refusal does
+                # not print "rebuilding..." for a table left untouched.
                 self.check_reachable(cursor, model)
+                self.stdout.write(f'{model._meta.label_lower}: rebuilding... ', ending='')
+                self.stdout.flush()
                 # populate_paths_sql() is the same SQL which backfilled these columns
                 # during the ltree migrations. It relies on SET LOCAL, so it must run
                 # inside a transaction, and the UPDATE it emits locks every row in the

netbox/utilities/mptt_to_ltree.py

@@ -35,8 +35,9 @@ ancestor `name` values. Keep the two modules in sync if either changes.
 
 __all__ = (
     'assert_paths_populated_sql',
-    'count_unreachable_rows_sql',
+    'count_stale_rows_sql',
     'populate_paths_sql',
+    'unreachable_rows_sql',
 )
 
 # Width to which each PK is zero-padded when used as an ltree label. Must match
@@ -118,10 +119,40 @@ UPDATE "{table}" SET path = t.path FROM t WHERE "{table}".id = t.id;
 """ + _RESTORE_SEARCH_PATH
 
 
-def count_unreachable_rows_sql(table):
+def count_stale_rows_sql(table, sort_path=False):
     """
-    Return SQL counting the rows in `table` which no root can reach by following
-    `parent_id`.
+    Return SQL counting the rows in `table` whose `path` disagrees with their parent's,
+    and (when `sort_path` is set) the rows whose `sort_path` does.
+
+    A reparent leaves `path` wrong, a rename leaves `sort_path` wrong, and while the
+    cascade trigger is missing either can happen without the other, so both are counted
+    separately.
+
+    This answers "does this table need rebuilding", not "how many rows are damaged". Only
+    a row which disagrees with its own parent is counted: the descendants below it agree
+    with their parents and are not, though they are equally stale. Treat any non-zero
+    result as the whole table needing a rebuild, and do not use it to decide which rows to
+    touch.
+    """
+    stale_sort_path = (
+        f'SELECT count(*) FROM "{table}" c JOIN "{table}" p ON c.parent_id = p.id'
+        f' WHERE c.sort_path <> p.sort_path || chr(9) || c.name'
+        if sort_path else 'SELECT 0'
+    )
+    return f"""
+SELECT
+    (
+        SELECT count(*) FROM "{table}" c JOIN "{table}" p ON c.parent_id = p.id
+        WHERE c.path <> p.path || lpad(c.id::text, {_PATH_LABEL_WIDTH}, '0')::ltree
+    ) AS stale_paths,
+    ({stale_sort_path}) AS stale_sort_paths;
+"""
+
+
+def unreachable_rows_sql(table, limit):
+    """
+    Return SQL reporting the rows in `table` which no root can reach by following
+    `parent_id`: how many there are, and the first `limit` of their ids.
 
     `populate_paths_sql()` seeds from `parent_id IS NULL` and walks downward, so it
     rewrites only the rows reachable that way. Anything else it leaves untouched, which
@@ -131,8 +162,10 @@ def count_unreachable_rows_sql(table):
 
     Callers which repair a populated table (rather than backfilling a fresh column, where
     `assert_paths_populated_sql()` catches the same condition via the NULLs left behind)
-    should run this first and refuse if it returns non-zero: the parent relationships have
-    to be corrected before any path rebuild can produce a correct answer.
+    should run this first and refuse if the count is non-zero: the parent relationships
+    have to be corrected before any path rebuild can produce a correct answer. The ids are
+    returned so that refusal can name rows to start from, rather than leaving the operator
+    to search the table for them.
     """
     return f"""
 WITH RECURSIVE reachable(id) AS (
@@ -140,7 +173,8 @@ WITH RECURSIVE reachable(id) AS (
     UNION ALL
     SELECT c.id FROM "{table}" c JOIN reachable r ON c.parent_id = r.id
 )
-SELECT count(*) FROM "{table}" t
+SELECT count(*), (array_agg(t.id ORDER BY t.id))[:{limit}]
+FROM "{table}" t
 WHERE NOT EXISTS (SELECT 1 FROM reachable r WHERE r.id = t.id);
 """
 

netbox/utilities/tests/test_management_commands.py

@@ -69,6 +69,27 @@ class RebuildLtreePathsTestCase(TestCase):
         cls.parent = Region.objects.create(name='Alpha', slug='alpha-rlp')
         cls.child = Region.objects.create(name='Beta', slug='beta-rlp', parent=cls.parent)
 
+    @staticmethod
+    def _set_parent_bypassing_triggers(pk, parent_pk):
+        """
+        Repoint a row's parent_id without firing the ltree triggers.
+
+        The BEFORE trigger recomputes `path` and rejects a move which its own cycle guard
+        can see, so the ORM cannot produce these states directly. Suppressing the triggers
+        for the statement reproduces what #23130 leaves behind: a database whose parent_id
+        graph has drifted from the paths stored alongside it.
+
+        `session_replication_role` is used rather than `ALTER TABLE ... DISABLE TRIGGER`,
+        which cannot run while the enclosing transaction has pending trigger events from
+        the rows created in setUpTestData.
+        """
+        with connection.cursor() as cursor:
+            cursor.execute("SET LOCAL session_replication_role = 'replica'")
+            cursor.execute(
+                'UPDATE dcim_region SET parent_id = %s WHERE id = %s', [parent_pk, pk]
+            )
+            cursor.execute("SET LOCAL session_replication_role = 'origin'")
+
     def test_rebuilds_stale_path_and_sort_path(self):
         Region.objects.filter(pk=self.child.pk).update(
             path='9999999999999999999', sort_path='stale',
@@ -104,41 +125,60 @@ class RebuildLtreePathsTestCase(TestCase):
             self.assertIn(label, output)
         self.assertIn('Finished.', output)
 
-    def test_rejects_a_model_which_is_not_hierarchical(self):
-        with self.assertRaises(CommandError):
-            call_command('rebuild_ltree_paths', 'dcim.site')
+    def test_check_reports_a_model_needing_a_rebuild(self):
+        Region.objects.filter(pk=self.child.pk).update(sort_path='stale')
+        out = StringIO()
 
-    @staticmethod
-    def _set_parent_bypassing_triggers(pk, parent_pk):
-        """
-        Repoint a row's parent_id without firing the ltree triggers.
+        call_command('rebuild_ltree_paths', 'dcim.region', '--check', stdout=out)
 
-        The BEFORE trigger recomputes `path` and rejects a move which its own cycle guard
-        can see, so the ORM cannot produce these states directly. Suppressing the triggers
-        for the statement reproduces what #23130 leaves behind: a database whose parent_id
-        graph has drifted from the paths stored alongside it.
+        output = out.getvalue()
+        self.assertIn('sort_path', output)
+        self.assertIn('Needs rebuilding: dcim.region', output)
 
-        `session_replication_role` is used rather than `ALTER TABLE ... DISABLE TRIGGER`,
-        which cannot run while the enclosing transaction has pending trigger events from
-        the rows created in setUpTestData.
-        """
-        with connection.cursor() as cursor:
-            cursor.execute("SET LOCAL session_replication_role = 'replica'")
-            cursor.execute(
-                'UPDATE dcim_region SET parent_id = %s WHERE id = %s', [parent_pk, pk]
-            )
-            cursor.execute("SET LOCAL session_replication_role = 'origin'")
+    def test_check_reports_a_healthy_model_as_ok(self):
+        out = StringIO()
+
+        call_command('rebuild_ltree_paths', 'dcim.region', '--check', stdout=out)
+
+        self.assertIn('dcim.region: OK', out.getvalue())
+        self.assertIn('Nothing to rebuild.', out.getvalue())
+
+    def test_check_modifies_nothing(self):
+        Region.objects.filter(pk=self.child.pk).update(sort_path='stale')
+
+        call_command('rebuild_ltree_paths', 'dcim.region', '--check', stdout=StringIO())
+
+        self.child.refresh_from_db()
+        self.assertEqual(self.child.sort_path, 'stale')
+
+    def test_check_reports_a_stale_path_where_sort_path_is_correct(self):
+        # A reparent leaves path wrong on its own, so the two counts are separate.
+        Region.objects.filter(pk=self.child.pk).update(path='9999999999999999999')
+        out = StringIO()
+
+        call_command('rebuild_ltree_paths', 'dcim.region', '--check', stdout=out)
+
+        self.assertIn('1 path', out.getvalue())
+
+    def test_rejects_a_model_which_is_not_hierarchical(self):
+        with self.assertRaises(CommandError):
+            call_command('rebuild_ltree_paths', 'dcim.site')
 
     def test_refuses_a_table_containing_a_cycle(self):
         """
         A rebuild walks down from the roots, so rows in a cycle are never reached and keep
-        whatever paths they have. Refuse rather than report success.
+        whatever paths they have. Refuse rather than report success, and name the rows to
+        start from: "correct the parent relationships" is not actionable without them.
         """
         self._set_parent_bypassing_triggers(self.parent.pk, self.child.pk)
 
-        with self.assertRaises(CommandError):
+        with self.assertRaises(CommandError) as ctx:
             call_command('rebuild_ltree_paths', 'dcim.region')
 
+        message = str(ctx.exception)
+        self.assertIn(str(self.parent.pk), message)
+        self.assertIn(str(self.child.pk), message)
+
     def test_refuses_a_table_containing_a_self_parented_row(self):
         self._set_parent_bypassing_triggers(self.child.pk, self.child.pk)