Code

Opened 3 years ago

Closed 12 months ago

#15117 closed Bug (fixed)

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

Reported by: akaihola Owned by: aaugustin
Component: Database layer (models, ORM) Version: master
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 d0ugal 3 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 3 years ago by russellm

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

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 Changed 3 years ago by jaddison

  • Severity set to Normal
  • Type set to Bug

comment:3 Changed 3 years ago by d0ugal

  • Cc dougal85@… added
  • Easy pickings unset

comment:4 Changed 3 years ago by sirmmo@…

  • Cc sirmmo@… added

Changed 3 years ago by d0ugal

comment:5 Changed 3 years ago by d0ugal

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 Changed 3 years ago by d0ugal

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 3 years ago by d0ugal (previous) (diff)

comment:7 Changed 3 years ago by d0ugal

  • milestone set to 1.4

comment:8 Changed 3 years ago by d0ugal

  • 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 Changed 3 years ago by andrewgodwin

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 follow-up: Changed 3 years ago by jacob

  • milestone 1.4 deleted

Milestone 1.4 deleted

comment:11 in reply to: ↑ 10 Changed 2 years ago by anonymous

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

TYIA :)

comment:12 Changed 14 months ago by aaugustin

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

comment:13 Changed 13 months ago by aaugustin

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 Changed 12 months ago by aaugustin

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 Changed 12 months ago by Aymeric Augustin <aymeric.augustin@…>

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

In 3a4276ffc35326f20e95890b50a67aebeabc9ad0:

Tested that get_or_create raises IntegrityError.

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

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.