diff --git a/docs/administration/management-commands.md b/docs/administration/management-commands.md index db5e11663..2989e766a 100644 --- a/docs/administration/management-commands.md +++ b/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 diff --git a/docs/release-notes/version-4.7.md b/docs/release-notes/version-4.7.md index d0b0de515..25496c9a6 100644 --- a/docs/release-notes/version-4.7.md +++ b/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 diff --git a/netbox/utilities/management/commands/rebuild_ltree_paths.py b/netbox/utilities/management/commands/rebuild_ltree_paths.py index d8c7f5288..60e8dccdc 100644 --- a/netbox/utilities/management/commands/rebuild_ltree_paths.py +++ b/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 diff --git a/netbox/utilities/mptt_to_ltree.py b/netbox/utilities/mptt_to_ltree.py index c613b8b90..ca2a75fb9 100644 --- a/netbox/utilities/mptt_to_ltree.py +++ b/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); """ diff --git a/netbox/utilities/tests/test_management_commands.py b/netbox/utilities/tests/test_management_commands.py index 49c1f8db4..46440cb5c 100644 --- a/netbox/utilities/tests/test_management_commands.py +++ b/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_check_reports_a_model_needing_a_rebuild(self): + Region.objects.filter(pk=self.child.pk).update(sort_path='stale') + out = StringIO() + + call_command('rebuild_ltree_paths', 'dcim.region', '--check', stdout=out) + + output = out.getvalue() + self.assertIn('sort_path', output) + self.assertIn('Needs rebuilding: dcim.region', output) + + 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') - @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_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)