Skip to content

Fixes #595: SET CONSTRAINTS before poly column removal on revert#596

Open
bctiemann wants to merge 2 commits into
mainfrom
595-poly-revert-pending-trigger
Open

Fixes #595: SET CONSTRAINTS before poly column removal on revert#596
bctiemann wants to merge 2 commits into
mainfrom
595-poly-revert-pending-trigger

Conversation

@bctiemann

@bctiemann bctiemann commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Fixes: #595

Summary

  • remove_polymorphic_object_columns() dropped the _content_type and _object_id columns with ALTER TABLE but did not flush deferred FK trigger events first.
  • On branch revert, the preceding row deletion queues deferred FK trigger events from the DEFERRABLE INITIALLY DEFERRED through-table FK (created by any MULTIOBJECT field on the same COT). PostgreSQL then rejects the ALTER TABLE with ObjectInUse: cannot ALTER TABLE "custom_objects_<id>" because it has pending trigger events.
  • Fix: issue SET CONSTRAINTS ALL IMMEDIATE before the first ALTER TABLE, mirroring what _schema_remove_field() already does for scalar fields.

Test plan

🤖 Generated with Claude Code

remove_polymorphic_object_columns() dropped the content_type and object_id
columns with ALTER TABLE but did not flush deferred FK trigger events first.
On branch revert, the preceding row deletion queues deferred FK trigger events
(from the DEFERRABLE INITIALLY DEFERRED through-table FK); the subsequent
ALTER TABLE is then rejected by PostgreSQL with "pending trigger events".

Mirror the fix already applied to _schema_remove_field for scalar fields:
issue SET CONSTRAINTS ALL IMMEDIATE before the first ALTER TABLE.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@bctiemann

Copy link
Copy Markdown
Contributor Author

@claude /review and check only for correctness.

@github-actions

github-actions Bot commented Jun 24, 2026

Copy link
Copy Markdown

Claude finished @bctiemann's task in 2m 29s —— View job


Code Review — Correctness

  • Read the diff and relevant code
  • Review for correctness
  • Post findings

The fix is correct.

What the change does

remove_polymorphic_object_columns() (field_types.py:945) drops the _content_type and _object_id columns from a custom object table. On the branch revert path, a prior row deletion queues deferred FK trigger events from a DEFERRABLE INITIALLY DEFERRED through-table FK (created by any MULTIOBJECT field on the same COT). PostgreSQL rejects the subsequent ALTER TABLE with ObjectInUse: cannot ALTER TABLE because it has pending trigger events.

The fix issues SET CONSTRAINTS ALL IMMEDIATE via the schema editor before the first remove_field call, causing PostgreSQL to fire the queued trigger events immediately, clearing the "in-use" block before the ALTER TABLE is attempted.

Correctness analysis

  1. Root cause is accurately identified. Django's PostgreSQL backend creates FK constraints as DEFERRABLE INITIALLY DEFERRED by default when calling schema_editor.create_model(). This causes trigger events to queue on row deletion rather than fire immediately.

  2. Migration 0011 (0011_non_deferrable_fk_constraints.py) is already on main and converts existing DEFERRABLE FKs to non-DEFERRABLE. However, it only covers constraints existing at migration time. Any new MULTIOBJECT through-table created after that migration runs will again have DEFERRABLE FKs. This PR's SET CONSTRAINTS ALL IMMEDIATE is therefore necessary as a forward-looking fix and correctly complements migration 0011.

  3. Placement is correct. The statement runs before either try block that calls schema_editor.remove_field(), so it clears the deferred queue unconditionally before any DDL is attempted.

  4. Side-effect is safe. SET CONSTRAINTS ALL IMMEDIATE scoped to the current transaction. If deferred events represent genuinely invalid FK references, it raises — which is the correct behavior (you do not want to proceed with schema alteration in an inconsistent state). In the revert scenario described, the rows have already been deleted so there is no integrity violation.

  5. No risk of double-firing. SET CONSTRAINTS ALL IMMEDIATE does not re-run constraints that have already been validated; it only flushes the pending queue.

  6. Minor gap: no automated regression test. The test suite has no coverage for the specific PostgreSQL error path (MULTIOBJECT field on same COT as OBJECT field during branch revert triggers "pending trigger events"). The test plan delegates this to a manual repro script from issue Reverting a merged branch fails for COTs with a polymorphic object field #595. Given the complexity of reproducing deferred-trigger behaviour in django.test.TestCase (which wraps each test in a transaction where deferred events behave differently), this is understandable — but worth noting.

Verdict

The change is a minimal, targeted, correct fix. No issues found with correctness.

@bctiemann bctiemann requested a review from arthanson June 24, 2026 22:08

@arthanson arthanson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should have a test for this scenario, should be easy to adapt from script in issue.

Adds ReferencedObjectDeletionTest.test_remove_poly_obj_columns_succeeds_with_pending_deferred_triggers
to exercise the fix from commit 7ddc34a.

The test reproduces the branching revert path:
  1. Delete through-table rows (simulates Collector cascade).
  2. Delete the COT instance via raw SQL → PostgreSQL queues a deferred
     trigger event on the referenced table (custom_objects_X).
  3. Call remove_polymorphic_object_columns() — without the fix this fails
     with "cannot ALTER TABLE … because it has pending trigger events";
     with the fix, SET CONSTRAINTS ALL IMMEDIATE fires and clears the
     pending event before the ALTER TABLE runs.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@bctiemann bctiemann requested a review from arthanson July 3, 2026 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reverting a merged branch fails for COTs with a polymorphic object field

2 participants