Opened 13 years ago

Closed 11 years ago

#15117 closed Bug (fixed)

get_or_create() raises "DatabaseError: no such savepoint" instead of IntegrityError (PostgreSQL)

Reported by: Antti Kaihola Owned by: Aymeric Augustin
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: dougal85@…, sirmmo@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Given the following models in a world app:

class Country(models.Model):
    name = models.CharField(max_length=50)

class CountryExtraInfo(models.Model):
    country = models.ForeignKey(Country, primary_key=True)
    info = models.TextField(blank=True, default='')

and assuming no Countries have been created yet, the following happens:

>>> from world.models import CountryExtraInfo
>>> CountryExtraInfo.objects.get_or_create(pk=1)
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "django/db/models/manager.py", line 135, in get_or_create
    return self.get_query_set().get_or_create(**kwargs)
  File "django/db/models/query.py", line 389, in get_or_create
    transaction.savepoint_rollback(sid, using=self.db)
  File "django/db/transaction.py", line 235, in savepoint_rollback
    connection._savepoint_rollback(sid)
  File "django/db/backends/__init__.py", line 70, in _savepoint_rollback
    self.cursor().execute(self.ops.savepoint_rollback_sql(sid))
  File "django/db/backends/postgresql_psycopg2/base.py", line 44, in execute
    return self.cursor.execute(query, args)
DatabaseError: no such savepoint

Also:

>>> from world.models import Country, CountryExtraInfo
>>> c = Country()
>>> c.id = 1
>>> CountryExtraInfo.objects.get_or_create(country=c)
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "django/db/models/manager.py", line 135, in get_or_create
    return self.get_query_set().get_or_create(**kwargs)
  File "django/db/models/query.py", line 389, in get_or_create
    transaction.savepoint_rollback(sid, using=self.db)
  File "django/db/transaction.py", line 235, in savepoint_rollback
    connection._savepoint_rollback(sid)
  File "django/db/backends/__init__.py", line 70, in _savepoint_rollback
    self.cursor().execute(self.ops.savepoint_rollback_sql(sid))
  File "django/db/backends/postgresql_psycopg2/base.py", line 44, in execute
    return self.cursor.execute(query, args)
DatabaseError: no such savepoint

In these situations, it would make more sense to get an IntegrityError instead.

Test environment:

  • Python 2.6.6
  • Django r15242
  • PostgreSQL 8.4.6
  • psycopg2 2.2.1

Attachments (1)

15117_get_or_create.patch (1.8 KB ) - added by Dougal Matthews 13 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 by Russell Keith-Magee, 13 years ago

Triage Stage: UnreviewedAccepted

Agreed; a savepoint error isn't the right response here. I'm not completely convinced that IntegrityError is right, either; get_or_create should be able to... well... get, or create. However, I can see that there are issues with that in practice.

comment:2 by James Addison, 13 years ago

Severity: Normal
Type: Bug

comment:3 by Dougal Matthews, 13 years ago

Cc: dougal85@… added
Easy pickings: unset

comment:4 by sirmmo@…, 13 years ago

Cc: sirmmo@… added

by Dougal Matthews, 13 years ago

Attachment: 15117_get_or_create.patch added

comment:5 by Dougal Matthews, 13 years ago

I've attached a patch with a failing test case to show the problem.

The problem here is only seen (from what I can tell and test) in ORM backends that support savepoints. Basically, when the integrity error is raised a rollback is attempted to the save point, however, it doesn't exist thus raising the "DatabaseError: no such savepoint". I can't figure out why the save point is lost, would it be rolled back automatically when the integrity error happens?

The following lines then in get_or_create actually re-raise the Integrity error and I think this should be the expected behaviour.

comment:6 by Dougal Matthews, 13 years ago

Another observation while I worked on this ticket: get_or_create has the following lines.

            except IntegrityError, e:
                transaction.savepoint_rollback(sid, using=self.db)
                try:
                    return self.get(**lookup), False
                except self.model.DoesNotExist:
                    raise e

Would it not be preferable to do the following to preserve the full original traceback? (Note the addition of a pass and then using "raise" rather than "raise e")

            except IntegrityError, e:
                transaction.savepoint_rollback(sid, using=self.db)
                try:
                    return self.get(**lookup), False
                except self.model.DoesNotExist:
                    pass
                raise

This is not directly related to the tickets issue, but to the more general problem of improving get_or_creates error reporting.

Last edited 13 years ago by Dougal Matthews (previous) (diff)

comment:7 by Dougal Matthews, 13 years ago

milestone: 1.4

comment:8 by Dougal Matthews, 13 years ago

UI/UX: unset

I turned on the log in postgres to see what was happening in the SQL.

2011-06-25 03:03:12 PDT LOG:  statement: SELECT "get_or_create_integrityerrortest"."country_id", "get_or_create_integrityerrortest"."info" FROM "get_or_create_integrityerrortest" WHERE "get_or_create_integrityerrortest"."country_id" = 1 
2011-06-25 03:03:12 PDT LOG:  statement: SAVEPOINT s1217009984_x1
2011-06-25 03:03:12 PDT LOG:  statement: INSERT INTO "get_or_create_integrityerrortest" ("country_id", "info") VALUES (1, E'')
2011-06-25 03:03:12 PDT LOG:  statement: COMMIT
2011-06-25 03:03:12 PDT ERROR:  insert or update on table "get_or_create_integrityerrortest" violates foreign key constraint "get_or_create_integrityerrortest_country_id_fkey"
2011-06-25 03:03:12 PDT DETAIL:  Key (country_id)=(1) is not present in table "get_or_create_person".
2011-06-25 03:03:12 PDT STATEMENT:  COMMIT
2011-06-25 03:03:12 PDT LOG:  statement: BEGIN; SET TRANSACTION ISOLATION LEVEL READ COMMITTED
2011-06-25 03:03:12 PDT LOG:  statement: ROLLBACK TO SAVEPOINT s1217009984_x1
2011-06-25 03:03:12 PDT ERROR:  no such savepoint
2011-06-25 03:03:12 PDT STATEMENT:  ROLLBACK TO SAVEPOINT s1217009984_x1
2011-06-25 03:03:12 PDT LOG:  statement: ROLLBACK

It appears that postgres has gotten rid of the savepoint.

comment:9 by Andrew Godwin, 13 years ago

So, I worked with Dougal on this at the Europython sprints, and the problem is that the code tries to COMMIT the previous transaction, which fails, and then tries to ROLLBACK TO SAVEPOINT after the commit has failed.

As far as I can tell, the failing COMMIT implicitly clears the savepoints in the previous transaction. The postgres client libraries reflect this by sending a BEGIN along with the ROLLBACK TO SAVEPOINT, as they assume the failing COMMIT cancels the previous transaction. This then causes the ROLLBACK TO SAVEPOINT to fail, as it's a new transaction without that savepoint.

We changed the backend to not send the BEGIN after the COMMIT, but then Postgres complained that this was invalid, and so I suspect it's not valid to send ROLLBACK TO SAVEPOINT _after_ COMMIT (all the examples in the postgres docs have it before a COMMIT).

It seems, then, that the "clean" fix would be to make this code just start a new transaction if its COMMIT fails, but that leads to potential nesting errors depending if the outer code has already started a transaction or not. I can't see a nice way to roll back to the savepoint at the moment, but my brain hurts after too much poking around with transactions, and I can't find the part of the SQL spec that Postgres is apparently adhering to in this regard (I think it's in SQL 1999, and I can't find where to download it).

comment:10 by Jacob, 12 years ago

milestone: 1.4

Milestone 1.4 deleted

in reply to:  10 comment:11 by anonymous, 12 years ago

What to do when in this situation where all get_or_create fail ?

TYIA :)

comment:12 by Aymeric Augustin, 11 years ago

Owner: changed from nobody to Aymeric Augustin
Status: newassigned

comment:13 by Aymeric Augustin, 11 years ago

I just introduced django.db.transaction.atomic that precisely designed for this use case.

It uses "release savepoint" instead of "commit", which allows rolling back to the savepoint if necessary.

comment:14 by Aymeric Augustin, 11 years ago

Indeed, this specific bug is fixed in master. I'm going to commit a variant of the test case attached by d0ugal.

The code provided by the reporter now raises an IntegrityError when the transaction is committed (immediately if no transaction is in progress) on databases that enforce integrity. It inserts bad data silently on other databases.

comment:15 by Aymeric Augustin <aymeric.augustin@…>, 11 years ago

Resolution: fixed
Status: assignedclosed

In 3a4276ffc35326f20e95890b50a67aebeabc9ad0:

Tested that get_or_create raises IntegrityError.

It used to raise "DatabaseError: no such savepoint" with the old
transaction management. Closes #15117.

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