Code

Opened 6 years ago

Closed 6 years ago

Last modified 3 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: master
Severity: Keywords:
Cc: richard.davies@… Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

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@…> 6 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@…> 6 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)

Changed 6 years ago by Richard Davies <richard.davies@…>

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

comment:1 Changed 6 years ago by mtredinnick

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to invalid
  • Status changed from new to closed

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 Changed 6 years ago by mtredinnick

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

Changed 6 years ago by Richard Davies <richard.davies@…>

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 Changed 6 years ago by mtredinnick

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

comment:4 Changed 3 years ago by jacob

  • milestone 1.0 deleted

Milestone 1.0 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.