Opened 16 years ago

Closed 16 years ago

Last modified 12 years ago

#8739 closed (invalid)

Transaction problems with save(force_insert=True) on postgresql (and likely other transactional dbs)

Reported by: Richard Davies <richard.davies@…> Owned by: nobody
Component: Database layer (models, ORM) Version: dev
Severity: Keywords:
Cc: richard.davies@… Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I attach a patch which adds an extra test, regressiontests/save_force_insert. This test succeeds with a SQLite backend, but fails with a postgreSQL backend. The postgreSQL error cites the line immediately after the call to save(force_insert=True), but I believe that this means that the transaction was aborted when save(force_insert=True) threw an exception, and that some form of savepoint protection may be needed inside the implementation of save().

$ ./runtests.py --settings=sqlite save_force_insert
----------------------------------------------------------------------
Ran 1 test in 0.005s

OK


$ ./runtests.py --settings=postgresql_psycopg2 save_force_insert
======================================================================
FAIL: Doctest: regressiontests.save_force_insert.models.__test__.API_TESTS
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/elastic/django-dev/trunk/django/test/_doctest.py", line 2180, in runTest
    raise self.failureException(self.format_failure(new.getvalue()))
AssertionError: Failed doctest test for regressiontests.save_force_insert.models.__test__.API_TESTS
  File "/home/elastic/django-dev/trunk/tests/regressiontests/save_force_insert/models.py", line unknown line number, in API_TESTS

----------------------------------------------------------------------
File "/home/elastic/django-dev/trunk/tests/regressiontests/save_force_insert/models.py", line ?, in regressiontests.save_force_insert.models.__test__.API_TESTS
Failed example:
    ManualPrimaryKeyTest.objects.get(id=1).data == 'Original'
Exception raised:
    Traceback (most recent call last):
      File "/home/elastic/django-dev/trunk/django/test/_doctest.py", line 1267, in __run
        compileflags, 1) in test.globs
      File "<doctest regressiontests.save_force_insert.models.__test__.API_TESTS[3]>", line 1, in <module>
        ManualPrimaryKeyTest.objects.get(id=1).data == 'Original'
      File "/home/elastic/django-dev/trunk/django/db/models/manager.py", line 81, in get
        return self.get_query_set().get(*args, **kwargs)
      File "/home/elastic/django-dev/trunk/django/db/models/query.py", line 296, in get
        num = len(clone)
      File "/home/elastic/django-dev/trunk/django/db/models/query.py", line 152, in __len__
        self._result_cache = list(self.iterator())
      File "/home/elastic/django-dev/trunk/django/db/models/query.py", line 267, in iterator
        for row in self.query.results_iter():
      File "/home/elastic/django-dev/trunk/django/db/models/sql/query.py", line 206, in results_iter
        for rows in self.execute_sql(MULTI):
      File "/home/elastic/django-dev/trunk/django/db/models/sql/query.py", line 1665, in execute_sql
        cursor.execute(sql, params)
    InternalError: current transaction is aborted, commands ignored until end of transaction block



----------------------------------------------------------------------
Ran 1 test in 0.012s

FAILED (failures=1)

Attachments (2)

save_force_insert.diff (1.2 KB ) - added by Richard Davies <richard.davies@…> 16 years ago.
Extra test case (note that the init.py must be created too)
8739_save_force_insert.v2.diff (1.3 KB ) - added by Richard Davies <richard.davies@…> 16 years ago.
Agreed with Malcolm that this ticket was a mistake. Before I sign off, here's a test case for force_insert functionality only, which _does_ do the transaction management itself, and succeeds on both databases

Download all attachments as: .zip

Change History (6)

by Richard Davies <richard.davies@…>, 16 years ago

Attachment: save_force_insert.diff added

Extra test case (note that the init.py must be created too)

comment:1 by Malcolm Tredinnick, 16 years ago

Resolution: invalid
Status: newclosed

The save() method won't do transaction management for you. If save() throws an IntegrityError, it's up to your code to deal with it. That's why force_insert are to be used extremely rarely (and why, for example, create() does the savepoint management).

So if I understand the ticket correctly -- which is that after raising an IntegrityError the tests must clean up and rollback -- the answer is, yes, that's correct. Intended behaviour.

If there's some other problem you're reporting here that I'm missing, please reopen with an alternate explanation. But Django isn't going to do your transaction management for you, since there's never a single correct solution.

comment:2 by Malcolm Tredinnick, 16 years ago

Err ... when I wrote create() above, I meant get_or_create().

by Richard Davies <richard.davies@…>, 16 years ago

Agreed with Malcolm that this ticket was a mistake. Before I sign off, here's a test case for force_insert functionality only, which _does_ do the transaction management itself, and succeeds on both databases

comment:3 by Malcolm Tredinnick, 16 years ago

As best I can tell, this test is duplicating one of the cases in tests/force_insert_update/models.py.

comment:4 by Jacob, 12 years ago

milestone: 1.0

Milestone 1.0 deleted

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