Refuse to rebuild ltree paths a root cannot reach
populate_paths_sql() seeds its recursive CTE from parent_id IS NULL and walks downward, so it rewrites only the rows some root reaches. Anything else it leaves alone, and rebuild_ltree_paths reported success regardless: the same shape of silent failure the command exists to repair. Three states put a row out of reach, and only one of them is a cycle: a cycle, a row parented to itself, and a parent_id referencing a row which no longer exists. All three are reachable in a database affected by #23130, because the triggers' own cycle guards compare against the stored path. Once the cascade trigger is missing those paths drift from the parent_id graph, and a move the guard would otherwise reject is accepted, closing a cycle. The check counts unreachable rows before the rebuild and raises CommandError naming the table, leaving the parent relationships for the operator to correct: only they can say what the hierarchy was meant to be. Each table is checked and rebuilt in one transaction. Sharing the transaction closes the window in which a concurrent write could strand a row between the check and the rebuild. Keeping it per-table rather than wrapping the whole run means a refusal rolls back only that table, and one table's row locks are not held while the remaining tables rebuild.
This commit is contained in:
Jason Novinger
parent
eb57625a91
commit
31b3053003
|
|
@ -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.
|
||||
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
||||
|
|
|
|||
|
|
@ -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())
|
||||
)
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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')
|
||||
|
|
|
|||
Loading…
Reference in New Issue