Explorar el Código

Correct what sharing a transaction gives the reachability check

The docstring claimed that running the check and the rebuild in one transaction
closes the window in which a concurrent write could strand a row. It does not:
under READ COMMITTED, which is the default, every statement takes a fresh
snapshot, so a reparent committed between the two is still missed.

Sharing the transaction is still worth doing, for reasons the docstring now
gives instead: a refusal rolls back cleanly, and populate_paths_sql() needs a
transaction for its SET LOCAL regardless. Neither is an isolation guarantee.

Say in the documentation that a rebuild assumes nothing else is changing the
hierarchy, which is what the command actually requires, alongside the existing
advice to run it in a maintenance window.
Jason Novinger hace 1 día
padre
commit
5eadef5499

+ 2 - 0
docs/administration/management-commands.md

@@ -96,6 +96,8 @@ One of the listed objects is in a cycle, parented to itself, or pointing at an o
 !!! warning
     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.
 
+    A rebuild also assumes nothing else is changing the hierarchy while it runs. An object reparented after the command has checked the model, but before it rewrites it, is not accounted for, and the check which refuses unreachable objects cannot see it either. This is another reason to run the command with writes paused rather than against a live system.
+
 ## rebuild_prefixes
 
 Rebuild the IPAM prefix hierarchy, recalculating the depth and child counts for all prefixes.

+ 5 - 3
netbox/utilities/management/commands/rebuild_ltree_paths.py

@@ -72,9 +72,11 @@ class Command(BaseCommand):
         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.
+        Takes the caller's cursor so a refusal rolls back with the transaction the rebuild
+        would have run in. That does not make the pair atomic with respect to other
+        writers: under READ COMMITTED every statement takes a fresh snapshot, so a
+        reparent committed between the check and the rebuild is still missed. Pause writes
+        for the duration, as the documentation says to.
         """
         cursor.execute(unreachable_rows_sql(model._meta.db_table, self.REPORTED_IDS))
         unreachable, ids = cursor.fetchone()