Opened 14 years ago
Closed 12 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)
Change History (16)
comment:1 by , 14 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → Bug |
comment:3 by , 14 years ago
Cc: | added |
---|---|
Easy pickings: | unset |
comment:4 by , 14 years ago
Cc: | added |
---|
by , 14 years ago
Attachment: | 15117_get_or_create.patch added |
---|
comment:5 by , 14 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 , 14 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.
comment:7 by , 14 years ago
milestone: | → 1.4 |
---|
comment:8 by , 14 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 , 14 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:11 by , 13 years ago
What to do when in this situation where all get_or_create fail ?
TYIA :)
comment:12 by , 12 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:13 by , 12 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 , 12 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 , 12 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
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.