Code

Opened 10 months ago

Closed 3 months ago

Last modified 3 months ago

#21166 closed Bug (fixed)

Reset the errors_occurred flag

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

Description

Currently it's reset only in close_if_unusable_or_obsolete, after making a test query.

I think it should be reset in atomic.__exit__ after a successful rollback too.

Reported by Anssi.

Attachments (0)

Change History (5)

comment:1 Changed 10 months ago by aaugustin

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

comment:2 Changed 3 months ago by aaugustin

In fact, I'm hesitating about this. It's a very small change: https://github.com/django/django/pull/2537.

errors_occurred is set after exceptions other than DataError and IntegrityError. Given the definition of these two exceptions, they're unlikely to be thrown in situations that make the connection unusable. I can't say much about the other exceptions. That's why I implemented a safety net in close_if_unusable_or_obsolete(). When another exception occurs, that method tests if the connection still works, and if it doesn't, drops it.

The vast majority of expected and recoverable exceptions are IntegrityErrors. These don't set the flag and don't incur the cost of the extra query to check the state of the connection. So there isn't a lot at stake.

But a small gain is still a gain. I believe that a successful ROLLBACK validates the state of the connection just as well as a SELECT 1. While we're there, a successful COMMIT also means that the connection works.

The argument against this change is to keep the safety net simple. Preserving a broken connection is very bad; making an extra query on a working connection is almost negligible.

Considering that the patch is dead simple, if I had to make the choice, I'd say +0.

comment:3 Changed 3 months ago by aaugustin

  • Has patch set

comment:4 Changed 3 months ago by Aymeric Augustin <aymeric.augustin@…>

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

In 3033a7193afcd1abdc0158e95ca3a241d8448d25:

Fixed #21166 -- Reset errors_occurred flag after commit and rollback.

comment:5 Changed 3 months ago by Aymeric Augustin <aymeric.augustin@…>

In 6b38e48ba11ec3d5aa433948d1354a8627c1edba:

[1.7.x] Fixed #21166 -- Reset errors_occurred flag after commit and rollback.

Backport of 3033a71 from master.

Conflicts:

django/db/backends/init.py

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.