2
0
Эх сурвалжийг харах

Address review feedback on the ltree trigger fix

- Assert on the presence of the `::text` cast rather than PostgreSQL's exact
  rendering of the WHEN clause, which is an implementation detail of its
  deparser and would need revisiting on a future major release.
- Correct 0251's docstring on locking: reinstalling a trigger takes ACCESS
  EXCLUSIVE, which is stronger than the ROW EXCLUSIVE held by 0242's backfill
  and blocks readers too. It is brief and scans nothing, but on a busy table it
  queues behind any long-running query.
- Correct 0251's docstring on reversal: reversing 0242 drops these triggers
  rather than recreating them. The no-op reverse is still right, since undoing
  a corrective reinstall has no target state of its own.
- Drop the release note's bug-fix bullet, which is generated in aggregate at
  release time and would conflict there. The operator warning stays: it is
  upgrade-time guidance that a changelog entry would not convey.
Jason Novinger 10 цаг өмнө
parent
commit
ec75e72c71

+ 0 - 4
docs/release-notes/version-4.7.md

@@ -25,10 +25,6 @@
 
     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.
 
-### Bug Fixes
-
-* [#23130](https://github.com/netbox-community/netbox/issues/23130) - Ensure the triggers which cascade hierarchical paths to descendants can be restored from a `pg_dump`
-
 ---
 
 ## v4.7.0 (2026-09-02)

+ 13 - 7
netbox/dcim/migrations/0251_fix_ltree_cascade_triggers.py

@@ -11,13 +11,19 @@ are absent) and one upgraded in place (they exist with the old definition, which
 fail its own next restore). InstallLtreeTriggers drops before creating, so this applies
 cleanly in either state.
 
-This reinstalls triggers only. It does not touch table data, so it does not incur the
-table-wide lock that 0242's path backfill did, and it does not repair path/sort_path
-values which went stale while the triggers were missing; see the v4.7.1 release notes for
-detection and repair.
-
-Reversing this migration is a no-op: the triggers it replaces belong to 0242_ltree_paths,
-which recreates them (from the corrected template) when reversed in turn.
+This reinstalls triggers only, so it takes ACCESS EXCLUSIVE on each table for the DDL
+itself and performs no table scan. Note that this is a stronger lock than the ROW
+EXCLUSIVE held by 0242's backfill, and it blocks readers as well as writers: it is brief,
+but on a busy table it queues behind any long-running query and holds everything behind
+it for that query's duration.
+
+It does not repair path/sort_path values which went stale while the triggers were
+missing; see the v4.7.1 release notes for detection and repair.
+
+Reversing this migration is a no-op. Reversing 0242_ltree_paths in turn drops these
+triggers rather than recreating them, which is that migration's business; what matters
+here is that undoing a corrective reinstall has no target state of its own, since the
+definition it replaced is the broken one.
 """
 from django.db import migrations
 

+ 8 - 1
netbox/utilities/tests/test_ltree.py

@@ -1102,12 +1102,19 @@ class CascadeTriggerDefinitionTests(TestCase):
             expected - set(definitions), set(),
             msg='these core ltree tables have no cascade trigger installed',
         )
+        # Assert on the cast rather than on PostgreSQL's exact rendering of the clause:
+        # the parenthesization pg_get_triggerdef() emits is an implementation detail.
         for table in sorted(expected):
+            definition = definitions[table]
             self.assertIn(
-                '(old.path)::text IS DISTINCT FROM (new.path)::text', definitions[table],
+                '::text IS DISTINCT FROM', definition,
                 msg=f'{table}: the cascade trigger compares ltree values directly, so it '
                     f'will not survive a pg_dump restore (see #23130)',
             )
+            self.assertNotRegex(
+                definition, r'old\.path\s+IS DISTINCT FROM\s+new\.path',
+                msg=f'{table}: the cascade trigger compares path without a cast to text',
+            )
 
 
 class LtreeTriggerSqlTests(SimpleTestCase):