Opened 6 years ago

Closed 2 years ago

#10813 closed Bug (fixed)

Database errors in the shell should roll back the transaction

Reported by: Glenn Owned by: aaugustin
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: fnl, diegobz Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

If an SQL statement fails in Postgresql, the connection refuses to run any further SQL commands until the transaction is rolled back. In normal operation, this is fine; but in the shell, it's a constant aggrevation.

This is related to #852, but that ticket was asking to automatically rollback inside the connection itself, which isn't good. This ticket is only regarding the shell. If I run a function that causes an SQL error, the shell's connection should not be left in an unusable state, forcing me to manually import db.transaction and roll back a transaction I never asked for. The shell should catch DatabaseError and rollback before returning to the console.

The attached patch implements this. I've only tested with Postgresql. This does not affect IPython (which I know nothing about).

Before:

>>> obj = User(name="Bill")
>>> obj.save()
...
IntegrityError: duplicate key value violates unique constraint "app_user_name_key"

>>> obj.name = "Jim"
>>> obj.save()
...
InternalError: current transaction is aborted, commands ignored until end of transaction block

Now:

>>> obj = User(name="Bill")
>>> obj.save()
...
IntegrityError: duplicate key value violates unique constraint "app_user_name_key"

>>> obj.name = "Jim"
>>> obj.save()
>>>

Attachments (1)

django-roll-back-after-db-error.diff (1.1 KB) - added by Glenn 6 years ago.

Download all attachments as: .zip

Change History (15)

Changed 6 years ago by Glenn

comment:1 Changed 6 years ago by Alex

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

comment:2 Changed 5 years ago by fnl

Any chance this issue will be 'fixed' in 1.2? This is a _very_ annoying problem.

comment:3 Changed 5 years ago by Glenn

I hope so. I'm sure most every Postgresql user of Django has been bitten by this.

comment:4 Changed 5 years ago by fnl

  • Cc fnl added

comment:5 Changed 4 years ago by diegobz

  • Cc diegobz added

comment:6 Changed 4 years ago by adrian

  • Patch needs improvement set
  • Severity set to Normal
  • Triage Stage changed from Design decision needed to Accepted
  • Type set to Uncategorized

I agree we should fix this, but I don't think the attached patch is the right solution. It appears to affect only manage.py shell, and a more bulletproof solution would affect any shell, including IPython and the normal python shell.

comment:7 follow-up: Changed 4 years ago by Glenn

In my opinion, this is ultimately a bug in Postgresql: errors should *not* prevent subsequent commands in a transaction from working. The only generic way I know to work around this is to set a savepoint before each command and commit it immediately after, so if an error occurs you can roll back to the checkpoint to unset the error flag. The overhead of that isn't acceptable in production, of course, and doing that only when in a shell (which I'm not even sure is possible) would be a debugging nightmare.

The correct fix is to convince Postgresql upstream to add a flag to disable this behavior, so errors within a transaction don't abort the entire transaction. This would make Postgresql's behavior match other SQL engines. I've had trouble with this many times over the years and have never had any luck getting them to fix this.

comment:8 Changed 4 years ago by julien

  • Type changed from Uncategorized to Bug

comment:9 Changed 4 years ago by gdufferin@…

  • Easy pickings unset
  • UI/UX unset

This bug also persists when creating a custom management task i.e /manage.py mycustomtask. Obviously, it would be ideal for database errors not to occur, thus not needing the rollback, but in some cases this is unavoidable, i.e in a try: Model.objects.get(*params) except: do something else...scenario.

Is there a fix?

comment:10 in reply to: ↑ 7 Changed 4 years ago by WoLpH

Replying to Glenn:

In my opinion, this is ultimately a bug in Postgresql: errors should *not* prevent subsequent commands in a transaction from working. The only generic way I know to work around this is to set a savepoint before each command and commit it immediately after, so if an error occurs you can roll back to the checkpoint to unset the error flag. The overhead of that isn't acceptable in production, of course, and doing that only when in a shell (which I'm not even sure is possible) would be a debugging nightmare.

Can you explain the rationale behind that? If _anything_ inside my transaction gives me an error which I had not anticipated (otherwise I should have used a savepoint) it should just rollback the entire transaction. Just allowing new commands like nothing has happened can easily cause data loss and goes directly against ACID compliance.

The correct fix is to convince Postgresql upstream to add a flag to disable this behavior, so errors within a transaction don't abort the entire transaction. This would make Postgresql's behavior match other SQL engines. I've had trouble with this many times over the years and have never had any luck getting them to fix this.

No, the correct fix is to make Django default to AutoCommit settings with Postgres. If you want to continue running commands after your transaction has died than you shouldn't be using transactions in the first place.

My proposed fix, make autocommit the default setting for postgres.

comment:11 follow-up: Changed 4 years ago by Glenn

Can you explain the rationale behind that? If _anything_ inside my transaction gives me an error which I had not anticipated (otherwise I should have used a savepoint) it should just rollback the entire transaction. Just allowing new commands like nothing has happened can easily cause data loss and goes directly against ACID compliance.

Nothing says that errors are unexpected. If an unexpected error does happen, it's the application's job to roll back the transaction; it's *not* the database's job to *force* it to.

No, the correct fix is to make Django default to AutoCommit? settings with Postgres. If you want to continue running commands after your transaction has died than you shouldn't be using transactions in the first place.

That doesn't make sense. Of course I should be using transactions.

My proposed fix, make autocommit the default setting for postgres.

The default behavior should not vary across databases.

I don't intend to debate this further, since this bug has sat around for well over three years (despite having a reasonable fix available for at least the most common and annoying problem), giving me little expectation of it getting fixed, and I'm not even using Django for anything right now.

comment:12 in reply to: ↑ 11 Changed 4 years ago by WoLpH

Replying to Glenn:

Nothing says that errors are unexpected. If an unexpected error does happen, it's the application's job to roll back the transaction; it's *not* the database's job to *force* it to.

Regardless, when the database gives you an error like this it can be impossible (e.g. with stored procedures, triggers, etc..) to fully understand what has happened behind the scenes.

The only reasonable cause of action in such cases is rolling back (either to a savepoint or the entire commit). There can be a lot of ambiguity if the database would allow you to continue after something like that. PostgreSQL has way too much features behind the scenes (rules, triggers, stored procedures) to allow you to fully know what is going on from Django.

If ambiguity is possible than there should be no option to commit after all. It can severely corrupt your database.

No, the correct fix is to make Django default to AutoCommit? settings with Postgres. If you want to continue running commands after your transaction has died than you shouldn't be using transactions in the first place.

That doesn't make sense. Of course I should be using transactions.

The principle of transactions is that you are sure that _all_ of your statements either commit succesfully or rollback. Not doing either of those goes against ACID.

My proposed fix, make autocommit the default setting for postgres.

The default behavior should not vary across databases.

But the default behaviour _is_ different. In MySQL the behaviour is very similar to autocommit mode by default. With MyISAM you don't even have transactions as an option. And having autocommit enabled doesn't stop you from manually opening a transaction like you would normally do.

comment:13 Changed 2 years ago by aaugustin

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

comment:14 Changed 2 years ago by Aymeric Augustin <aymeric.augustin@…>

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

In 14cddf51c5f001bb426ce7f7a83fdc52c8d8aee9:

Merged branch 'database-level-autocommit'.

Fixed #2227: atomic supports nesting.
Fixed #6623: commit_manually is deprecated and atomic doesn't suffer from this defect.
Fixed #8320: the problem wasn't identified, but the legacy transaction management is deprecated.
Fixed #10744: the problem wasn't identified, but the legacy transaction management is deprecated.
Fixed #10813: since autocommit is enabled, it isn't necessary to rollback after errors any more.
Fixed #13742: savepoints are now implemented for SQLite.
Fixed #13870: transaction management in long running processes isn't a problem any more, and it's documented.
Fixed #14970: while it digresses on transaction management, this ticket essentially asks for autocommit on PostgreSQL.
Fixed #15694: atomic supports nesting.
Fixed #17887: autocommit makes it impossible for a connection to stay "idle of transaction".

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