diff --git a/docs/administration/management-commands.md b/docs/administration/management-commands.md index b49dbf2fa..db5e11663 100644 --- a/docs/administration/management-commands.md +++ b/docs/administration/management-commands.md @@ -54,6 +54,8 @@ Pass one or more models as `app_label.ModelName` to limit the rebuild. 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. + !!! 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. diff --git a/docs/release-notes/version-4.7.md b/docs/release-notes/version-4.7.md index a6137ae71..5ddf085c0 100644 --- a/docs/release-notes/version-4.7.md +++ b/docs/release-notes/version-4.7.md @@ -27,7 +27,7 @@ python netbox/manage.py rebuild_ltree_paths dcim.region ``` - A rebuild rewrites every row of the named tables, locking those rows until it commits, so run it during a maintenance window. + A rebuild rewrites every row of the named tables, locking those rows until it commits, so run it during a maintenance window. Should it report that a table contains rows unreachable from any root, the parent relationships themselves need correcting first: a rebuild walks down from the roots and would skip those rows. Plugins which maintain their own `ltree` models via the `InstallLtreeTriggers` migration operation are affected in the same way, and their tables are not touched by the migrations above. Where such a database was restored from a dump, the plugin's cascade triggers are missing entirely; where it was upgraded in place, they carry the old definition and will be lost by its next dump. Either way, applying `InstallLtreeTriggers` again from a new plugin migration reinstalls them: as of this release the operation drops each trigger before recreating it, so it is safe to re-run. diff --git a/netbox/utilities/management/commands/rebuild_ltree_paths.py b/netbox/utilities/management/commands/rebuild_ltree_paths.py index 24ae4907e..d8c7f5288 100644 --- a/netbox/utilities/management/commands/rebuild_ltree_paths.py +++ b/netbox/utilities/management/commands/rebuild_ltree_paths.py @@ -4,7 +4,7 @@ from django.db import connection, transaction from netbox.models.ltree import LtreeModel from netbox.plugins import PluginConfig -from utilities.mptt_to_ltree import populate_paths_sql +from utilities.mptt_to_ltree import count_unreachable_rows_sql, populate_paths_sql class Command(BaseCommand): @@ -21,7 +21,8 @@ class Command(BaseCommand): def get_models(self, names): """ - Return the concrete core hierarchical models to operate on, ordered by table name. + Return the concrete core hierarchical models to operate on: those named, in the + order given, or every one of them ordered by table name. Plugin models are excluded, including when named explicitly: the SQL which rebuilds `sort_path` reads the name column by name, while `InstallLtreeTriggers` lets a plugin @@ -50,14 +51,45 @@ class Command(BaseCommand): models.append(model) return models + def check_reachable(self, cursor, model): + """ + Raise unless every row is reachable from a root by following `parent_id`. + + The rebuild walks down from `parent_id IS NULL`, so a row no root can reach is one + it silently leaves alone. Reporting success in that case would be the same failure + this command exists to repair: an operation which appears to have worked while the + data is still wrong. Refuse instead, and leave correcting the parent relationships + to the operator, since only they can say what the intended hierarchy was. + + Takes the caller's cursor to keep it visible that this must run in the same + 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] + + if unreachable: + 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'Correct the parent relationships, then re-run.' + ) + def handle(self, *args, **options): + # 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']): - # 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 table until it commits. self.stdout.write(f'{model._meta.label_lower}: rebuilding... ', ending='') self.stdout.flush() with transaction.atomic(), connection.cursor() as cursor: + self.check_reachable(cursor, model) + # 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 + # table until it commits. cursor.execute( populate_paths_sql(model._meta.db_table, sort_path=model._has_sort_path()) ) diff --git a/netbox/utilities/mptt_to_ltree.py b/netbox/utilities/mptt_to_ltree.py index 2eb19084b..c613b8b90 100644 --- a/netbox/utilities/mptt_to_ltree.py +++ b/netbox/utilities/mptt_to_ltree.py @@ -35,6 +35,7 @@ ancestor `name` values. Keep the two modules in sync if either changes. __all__ = ( 'assert_paths_populated_sql', + 'count_unreachable_rows_sql', 'populate_paths_sql', ) @@ -117,6 +118,33 @@ UPDATE "{table}" SET path = t.path FROM t WHERE "{table}".id = t.id; """ + _RESTORE_SEARCH_PATH +def count_unreachable_rows_sql(table): + """ + Return SQL counting the rows in `table` which no root can reach by following + `parent_id`. + + `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 + makes an unreachable row an unrepaired one. Three shapes cause it: a cycle, a row + whose `parent_id` is its own id, and a `parent_id` referencing a row which does not + exist. + + 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. + """ + return f""" +WITH RECURSIVE reachable(id) AS ( + SELECT id FROM "{table}" WHERE parent_id IS NULL + UNION ALL + SELECT c.id FROM "{table}" c JOIN reachable r ON c.parent_id = r.id +) +SELECT count(*) FROM "{table}" t +WHERE NOT EXISTS (SELECT 1 FROM reachable r WHERE r.id = t.id); +""" + + def assert_paths_populated_sql(table): """ Return SQL that raises if any row in `table` still has a NULL `path` after diff --git a/netbox/utilities/tests/test_management_commands.py b/netbox/utilities/tests/test_management_commands.py index 7bc4663db..49c1f8db4 100644 --- a/netbox/utilities/tests/test_management_commands.py +++ b/netbox/utilities/tests/test_management_commands.py @@ -3,9 +3,11 @@ from unittest.mock import MagicMock, patch from django.core.management import call_command from django.core.management.base import CommandError +from django.db import connection from django.test import TestCase from dcim.models import Region +from tenancy.models import TenantGroup from utilities.management.commands.calculate_cached_counts import Command @@ -105,3 +107,70 @@ class RebuildLtreePathsTestCase(TestCase): 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. + """ + self._set_parent_bypassing_triggers(self.parent.pk, self.child.pk) + + with self.assertRaises(CommandError): + call_command('rebuild_ltree_paths', 'dcim.region') + + def test_refuses_a_table_containing_a_self_parented_row(self): + self._set_parent_bypassing_triggers(self.child.pk, self.child.pk) + + with self.assertRaises(CommandError): + call_command('rebuild_ltree_paths', 'dcim.region') + + def test_refuses_a_table_whose_parent_id_references_a_missing_row(self): + # Not a cycle, but equally unreachable, so a cycle-specific check would miss it. + self._set_parent_bypassing_triggers(self.child.pk, self.parent.pk + 10000) + + with self.assertRaises(CommandError): + call_command('rebuild_ltree_paths', 'dcim.region') + + def test_refusing_a_table_leaves_that_table_untouched(self): + """ + A refusal rolls back the transaction it was raised in, so the refused table keeps + the paths it had. Tables already rebuilt stay rebuilt: each is its own transaction, + which is what keeps one table's row locks from being held while the rest run. + """ + group = TenantGroup.objects.create(name='Unrelated', slug='unrelated-rlp') + TenantGroup.objects.filter(pk=group.pk).update(sort_path='stale') + Region.objects.filter(pk=self.child.pk).update(sort_path='also-stale') + # dcim.region is named second and is the table which fails the check. + self._set_parent_bypassing_triggers(self.parent.pk, self.child.pk) + + with self.assertRaises(CommandError): + call_command('rebuild_ltree_paths', 'tenancy.tenantgroup', 'dcim.region') + + # The refused table is untouched: no partial rebuild, nothing to undo by hand. + self.child.refresh_from_db() + self.assertEqual(self.child.sort_path, 'also-stale') + + # The table which passed its own check was rebuilt and committed. + group.refresh_from_db() + self.assertEqual(group.sort_path, 'Unrelated')