Opened 21 months ago

Closed 20 months ago

Last modified 20 months ago

#20666 closed Bug (fixed)

Circularly dependent fixtures fail due to transaction committing on each object save

Reported by: yoyoma Owned by: aaugustin
Component: Database layer (models, ORM) Version: 1.6-alpha-1
Severity: Release blocker Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

When using manage.py loaddata to load a fixture that contains 2 objects that refer to each other circularly via foreign keys you'll encounter an integrity error. This is caused by the use of transaction.commit_on_success_unless_managed in the loaddata management command combined with the use of it in Model.save_base, which after each save leads to a transaction commit.

When I modified the loaddata management command to use transaction.atomic in lieu of transaction.commit_on_success_unless_managed the issue no longer occurred. I realize that isn't the solution (for backwards compatibility reasons), but it might help in debugging. Also, see the printed stack frames and debugging values in this post (full thread here).

This affects at least Django 1.6a1. A similar exception was encountered with Django 1.5.1, though something was subtly different about the issue in Django 1.5.1 (wasn't provided a stack trace to verify it was the same issue).

Attachments (1)

20666.patch (3.3 KB) - added by aaugustin 20 months ago.

Download all attachments as: .zip

Change History (11)

comment:1 follow-up: Changed 21 months ago by anonymous

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

I have the same problem in Django 1.6a1, but fixtures were loading without any problem in 1.5.1 and before. Something commits transactions for every object inserted, instead of committing once for all.

comment:2 Changed 21 months ago by anonymous

And, as stated, transaction.atomic fixes the loading of fixtures.

comment:3 in reply to: ↑ 1 Changed 21 months ago by yoyoma

Replying to anonymous:

I have the same problem in Django 1.6a1, but fixtures were loading without any problem in 1.5.1 and before. Something commits transactions for every object inserted, instead of committing once for all.

I actually ran into the problem with 1.5.1 as well, but I didn't take the time to figure out whether the cause was exactly the same (the error was from a different field, which threw me off).

comment:4 Changed 21 months ago by aaugustin

  • Owner changed from nobody to aaugustin
  • Status changed from new to assigned

comment:5 Changed 20 months ago by aaugustin

This ticket is a follow-up to this mailing list discussion: https://groups.google.com/forum/#!topic/django-developers/2_xR_PKeubs

First, I tried reproducing the issue with the models in the first post of the thread, but they don't even validate:

fixtures_regress.collection: Accessor for field 'main_thing' clashes with field 'Thing.collection'. Add a related_name argument to the definition for 'main_thing'.
fixtures_regress.collection: Reverse query name for field 'main_thing' clashes with field 'Thing.collection'. Add a related_name argument to the definition for 'main_thing'.

Then, I tried with the models and fixture provided in the third post of the thread, and I cannot reproduce the issue. I'm attaching the diff corresponding to that test.

If someone could provide a *reproducible* way to trigger the problem, in the form of a patch adding a failing test case to Django's test suite, it would really help.

Changed 20 months ago by aaugustin

comment:6 Changed 20 months ago by aaugustin

While I failed to reproduce the issue, I can see how d04964e70d5c6277e79874ba8950e23c224b323b would cause it.

If you're encountering the problem, can you apply this patch to Django and tell me if it helps:

diff --git a/django/core/management/commands/loaddata.py b/django/core/management/commands/loaddata.py
index 6856e85..f3f68f4 100644
--- a/django/core/management/commands/loaddata.py
+++ b/django/core/management/commands/loaddata.py
@@ -51,7 +51,7 @@ class Command(BaseCommand):
 
         self.verbosity = int(options.get('verbosity'))
 
-        with transaction.commit_on_success_unless_managed(using=self.using):
+        with transaction.atomic(using=self.using):
             self.loaddata(fixture_labels)
 
         # Close the DB connection -- unless we're still in a transaction. This

Strictly speaking, it's backwards-incompatible for users who have pre_save or post_save signals that invoke transaction.autocommit/commit_on_success/commit_manually, because the deprecated transaction management APIs cannot be invoked inside transaction.atomic.

Now, I'm not sure it's the right level to fix this bug. It's possible that commit_on_success_unless_managed itself is the source of the problem.

comment:7 Changed 20 months ago by aaugustin

  • Has patch set
  • Needs tests set
  • Severity changed from Normal to Release blocker
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Bug

After reviewing the implementation of the new transaction management, I think this issue is specific to loaddata, and that it should use transaction.atomic as shown above.

commit_on_success was used to begin a transaction, and hopefully nothing else would commit it, but that wasn't guaranteed at all. That was a bug, and it was revealed by my changes.

(I'm setting the "has patch" flag, that's referring to the diff shown in the previous comment.)

Last edited 20 months ago by aaugustin (previous) (diff)

comment:8 Changed 20 months ago by aaugustin

After re-reading the original thread I finally found out the cause. commit_on_success_unless_managed didn't ensure that the database connection was established before testing the autocommit status. When running loaddata, the connection isn't opened yet, and commit_on_success_unless_managed mistakenly believes it's already in a transaction.

Scratch everything I've written above.

I'm going to introduce a getter to test the autocommit status rather than access the autocommit flag directly. For consistency I'll do the same for the rollback flag.

This is hard to test, because the problem only happens when there isn't a connection to the database... and that's why I could never reproduce the problem described here from Django's test suite...

Last edited 20 months ago by aaugustin (previous) (diff)

comment:9 Changed 20 months ago by Aymeric Augustin <aymeric.augustin@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In dd9c6bc359a799fcbed647055b596239956a472a:

Introduced getters for connection.autocommit and .needs_rollback.

They ensure that the attributes aren't accessed in conditions where they
don't contain a valid value.

Fixed #20666.

comment:10 Changed 20 months ago by Aymeric Augustin <aymeric.augustin@…>

In 02976a46c926880e78eb64424a9c8aa28d8be48a:

[1.6.x] Introduced getters for connection.autocommit and .needs_rollback.

They ensure that the attributes aren't accessed in conditions where they
don't contain a valid value.

Fixed #20666.

Backport of dd9c6bc359a799fcbed647055b596239956a472a from master.

Note: See TracTickets for help on using tickets.
Back to Top