Opened 8 years ago

Closed 4 years ago

#6669 closed Uncategorized (fixed)

@transaction.commit_on_success does not rollback when it should

Reported by: james@… Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords: transaction, commit
Cc: shai, bhuztez@…, k@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I am using PostgresSQL and have a problem with code like this:

@transaction.commit_on_success
def create_system_user():
    user = User.objects.create_user(username='system', password='iamr00t', email='root@SITENAME.com')
    UserProfile.objects.create(user=user)

try:
    create_system_user()
except:
    pass

try:
    create_system_user()
except:
    pass

print User.objects.all()

the above code will give:

psycopg2.ProgrammingError: current transaction is aborted, commands ignored until end of transaction block

I have determined that adding a
transaction.rollback()
before the

print User.objects.all()

will allow the user list to be printed correctly.

Looking at the django code, I see:

            except Exception, e:
                if is_dirty():
                    rollback()
                raise

in commit_on_success. My guess is that is_dirty() does not take into consideration that an error in PostgresSQL will require a rollback before further work can continue.

Attachments (2)

6669-test-only.diff (1.4 KB) - added by claudep 4 years ago.
Test showing failure
6669-2.diff (2.4 KB) - added by claudep 4 years ago.
Set connection dirty even with CursorDebugWrapper

Download all attachments as: .zip

Change History (14)

comment:1 Changed 7 years ago by Simon Greenhill

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 follow-up: Changed 7 years ago by shai

Could this be related to #9964 ?

comment:3 Changed 5 years ago by dbenamy@…

I hit this problem too. Is this the intended behavior? It seems other people have hit this- http://www.mail-archive.com/django-users@googlegroups.com/msg99702.html

comment:4 in reply to: ↑ 2 Changed 5 years ago by shai

Replying to myself:

Could this be related to #9964 ?

It seems so; the patch which I uploaded there seems to fix this one too. If the patch is accepted, it may be adivsable to add to tests/regression_tests/transaction_regress/tests.py (a file created in that patch) the following lines:

    def test_6669(self):

        from django.contrib.auth.models import User

        @transaction.commit_on_success
        def create_system_user():
            user = User.objects.create_user(username='system', password='iamr00t', email='root@SITENAME.com')
            Mod.objects.create(fld=user.id)

        try:
            create_system_user()
        except:
            pass

        try:
            create_system_user()
        except:
            pass

        print User.objects.all()

comment:5 Changed 5 years ago by shai

  • Cc shai added

comment:6 Changed 5 years ago by shai

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

Fixed by [15493] (a fix for #9964, based on the patch mentioned in my earlier comment, and including the test quoted above).

comment:7 Changed 4 years ago by bhuztez

  • Cc bhuztez@… added
  • Easy pickings unset
  • Resolution fixed deleted
  • Severity set to Normal
  • Status changed from closed to reopened
  • Type set to Uncategorized
  • UI/UX unset

I use Django 1.3.1, but still have to rollback

>>> import django
>>> django.get_version()
'1.3.1'
>>> from django.conf import settings
>>> settings.DATABASES['default']['ENGINE']
'django.db.backends.postgresql_psycopg2'
>>> from django.db import transaction
>>> from django.contrib.auth.models import User
>>> @transaction.commit_on_success
... def create_system_user():
...     user = User.objects.create_user(username='system', password='iamr00t', email='root@SITENAME.com')
... 
>>> create_system_user()
>>> create_system_user()
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/usr/lib/python2.7/site-packages/django/db/transaction.py", line 217, in inner
    res = func(*args, **kwargs)
  File "<console>", line 3, in create_system_user
  File "/usr/lib/python2.7/site-packages/django/contrib/auth/models.py", line 136, in create_user
    user.save(using=self._db)
  File "/usr/lib/python2.7/site-packages/django/db/models/base.py", line 460, in save
    self.save_base(using=using, force_insert=force_insert, force_update=force_update)
  File "/usr/lib/python2.7/site-packages/django/db/models/base.py", line 553, in save_base
    result = manager._insert(values, return_id=update_pk, using=using)
  File "/usr/lib/python2.7/site-packages/django/db/models/manager.py", line 195, in _insert
    return insert_query(self.model, values, **kwargs)
  File "/usr/lib/python2.7/site-packages/django/db/models/query.py", line 1436, in insert_query
    return query.get_compiler(using=using).execute_sql(return_id)
  File "/usr/lib/python2.7/site-packages/django/db/models/sql/compiler.py", line 791, in execute_sql
    cursor = super(SQLInsertCompiler, self).execute_sql(None)
  File "/usr/lib/python2.7/site-packages/django/db/models/sql/compiler.py", line 735, in execute_sql
    cursor.execute(sql, params)
  File "/usr/lib/python2.7/site-packages/django/db/backends/util.py", line 34, in execute
    return self.cursor.execute(sql, params)
  File "/usr/lib/python2.7/site-packages/django/db/backends/postgresql_psycopg2/base.py", line 44, in execute
    return self.cursor.execute(query, args)
IntegrityError: duplicate key value violates unique constraint "auth_user_username_key"
DETAIL:  Key (username)=(system) already exists.

>>> User.objects.all()
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/usr/lib/python2.7/site-packages/django/db/models/query.py", line 69, in __repr__
    data = list(self[:REPR_OUTPUT_SIZE + 1])
  File "/usr/lib/python2.7/site-packages/django/db/models/query.py", line 84, in __len__
    self._result_cache.extend(self._iter)
  File "/usr/lib/python2.7/site-packages/django/db/models/query.py", line 273, in iterator
    for row in compiler.results_iter():
  File "/usr/lib/python2.7/site-packages/django/db/models/sql/compiler.py", line 680, in results_iter
    for rows in self.execute_sql(MULTI):
  File "/usr/lib/python2.7/site-packages/django/db/models/sql/compiler.py", line 735, in execute_sql
    cursor.execute(sql, params)
  File "/usr/lib/python2.7/site-packages/django/db/backends/util.py", line 34, in execute
    return self.cursor.execute(sql, params)
  File "/usr/lib/python2.7/site-packages/django/db/backends/postgresql_psycopg2/base.py", line 44, in execute
    return self.cursor.execute(query, args)
DatabaseError: current transaction is aborted, commands ignored until end of transaction block

>>> transaction.rollback()
>>> User.objects.all()
[<User: system>]

comment:8 Changed 4 years ago by charettes

I can also reproduce bhuztez's failing issue on trunk (rev 17095)

comment:9 Changed 4 years ago by k@…

  • Cc k@… added

Am seeing this problem with Django 1.3.1, and a non-IntegrityError exception in a view (wrapped with commit_on_success); an object created early on in the view is left in the database, after an error later in the view.

comment:10 Changed 4 years ago by claudep

  • Needs tests set

I guess this might be related to the settings.DEBUG state. When DEBUG is False, The CursorWrapper is setting the transaction dirty and the behaviour is correct. However, it seems that CursorDebugWrapper is not setting the transaction dirty (__getattr__ is not called as the execute method does exist on the class), hence the rollback does not happen in the decorator.

Changed 4 years ago by claudep

Test showing failure

Changed 4 years ago by claudep

Set connection dirty even with CursorDebugWrapper

comment:11 Changed 4 years ago by claudep

  • Has patch set
  • Needs tests unset

comment:12 Changed 4 years ago by aaugustin

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

In [17368]:

Fixed #6669 -- Ensured database connections are marked as dirty by CursorDebugWrapper.execute/executemany. Refs #9964. Thanks james at 10gic net for the report and Claude Paroz for the patch.

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