Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix issue #230 -- do not check isolation level in aborted transactions #296

Merged
merged 4 commits into from

8 participants

@akaariai

On PostgreSQL when the transaction is in aborted status, checking
connection.isolation_level will result in error because doing queries
in aborted transactions is illegal. Fixed this by skipping the
isolation level check when the transaction is aborted.

@akaariai

Looking at this it might be better to just do:

try:
    iso_level = conn.isolation_level:
except DatabaseError:
    iso_level = 'unknown'

in there.

@jezdez
Owner

Yeah, let's go with the simpler version, @akaariai

@jezdez
Owner

@akaariai any news on that?

@travisbot

This pull request passes (merged 821fbed7 into ad15287).

@travisbot

This pull request passes (merged e806603 into ad15287).

@akaariai

I had forgotten all about this. I modified the patch to do the try-except thing. In addition, there is a test in [akaariai@d9e9bf3] but I didn't figure how to test this properly (without modifying the main settings), so this is non-committable.

@akaariai akaariai Fixed issue #230 -- Avoid queries in aborted transactions
On PostgreSQL when the transaction is in aborted status, checking
connection.isolation_level will result in an error.
e806603
@jezdez
Owner

@akaariai we could probably run the tests on postgres by extending the travis configuration and skip the test if the configured database isn't a postgres db. what do you think?

@akaariai

I updated the patch so that running tests using "DJANGO_SETTINGS_MODULE=test_pgsql python runtests.py" is now possible. Along that path I had to fix another issue when using connections with alias not in django.db.connections.

@travisbot

This pull request passes (merged 54f4f3a into ad15287).

@nuklea

Good request!

@catalanojuan

Any news on this? Is this going to be merged any time soon? Thanks!

@johtso

Got bitten by this issue the other day, would be great if a fix could be merged :thumbsup:

@jlovison

I also just got bit by this issue. I also think #351 is because of this.

Does DatabaseError also catch IntegrityError?

Otherwise it should probably read except (DatabaseError, IntegrityError):

@jezdez jezdez merged commit 54f4f3a into django-debug-toolbar:master
@sturmf

Hmm, I hate to have to write, me too. But any reason this got not merged yet? Can we help in any way on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Aug 27, 2012
  1. @akaariai

    Fixed issue #230 -- Avoid queries in aborted transactions

    akaariai authored
    On PostgreSQL when the transaction is in aborted status, checking
    connection.isolation_level will result in an error.
  2. @akaariai

    Tests for issue_230

    akaariai authored
  3. @akaariai

    Made possible to use DJANGO_SETTINGS_MODULE in testing

    akaariai authored
    Also added a dummy test_pgsql settings file.
  4. @akaariai
This page is out of date. Refresh to see the latest.
View
19 debug_toolbar/utils/tracking/db.py
@@ -88,6 +88,14 @@ def execute(self, sql, params=()):
try:
return self.cursor.execute(sql, params)
finally:
+ # FIXME: Sometimes connections which are not in the connections
+ # dict are used (for example in test database destroying).
+ # The code below (at least get_transaction_id(alias) needs to have
+ # the connection in the connections dict. It would be good to
+ # not have this requirement at all, but for now lets just skip
+ # these connections.
+ if self.db.alias not in connections:
+ return
stop = datetime.now()
duration = ms_from_timedelta(stop - start)
enable_stacktraces = getattr(settings,
@@ -119,7 +127,7 @@ def execute(self, sql, params=()):
del cur_frame
alias = getattr(self.db, 'alias', 'default')
- conn = connections[alias].connection
+ conn = self.db.connection
# HACK: avoid imports
if conn:
engine = conn.__class__.__module__.split('.', 1)[0]
@@ -146,10 +154,17 @@ def execute(self, sql, params=()):
}
if engine == 'psycopg2':
+ # If an erroneous query was ran on the connection, it might
+ # be in a state where checking isolation_level raises an
+ # exception.
+ try:
+ iso_level = conn.isolation_level
+ except conn.InternalError:
+ iso_level = 'unknown'
params.update({
'trans_id': self.logger.get_transaction_id(alias),
'trans_status': conn.get_transaction_status(),
- 'iso_level': conn.isolation_level,
+ 'iso_level': iso_level,
'encoding': conn.encoding,
})
View
5 runtests.py
@@ -1,11 +1,14 @@
#!/usr/bin/env python
import sys
+import os
from os.path import dirname, abspath
from optparse import OptionParser
from django.conf import settings, global_settings
-if not settings.configured:
+# For convenience configure settings if they are not pre-configured or if we
+# haven't been provided settings to use by environment variable.
+if not settings.configured and not os.environ.get('DJANGO_SETTINGS_MODULE'):
settings.configure(
DATABASES={
'default': {
View
28 test_pgsql.py
@@ -0,0 +1,28 @@
+from django.conf import global_settings
+DATABASES={
+ 'default': {
+ 'ENGINE': 'django.db.backends.postgresql_psycopg2',
+ # Edit the below settings before use...
+ 'USER': '',
+ 'NAME': '',
+ 'HOST': '',
+ 'PASSWORD': '',
+ }
+}
+INSTALLED_APPS=[
+ 'django.contrib.auth',
+ 'django.contrib.admin',
+ 'django.contrib.contenttypes',
+ 'django.contrib.sessions',
+ 'django.contrib.sites',
+
+ 'debug_toolbar',
+
+ 'tests',
+]
+MIDDLEWARE_CLASSES=global_settings.MIDDLEWARE_CLASSES + (
+ 'debug_toolbar.middleware.DebugToolbarMiddleware',
+)
+ROOT_URLCONF=''
+DEBUG=False
+SITE_ID=1
View
15 tests/tests.py
@@ -2,9 +2,11 @@
from django.conf import settings
from django.contrib.auth.models import User
+from django.db import connection
from django.http import HttpResponse
from django.test import TestCase, RequestFactory
from django.template import Template, Context
+from django.utils import unittest
from debug_toolbar.middleware import DebugToolbarMiddleware
from debug_toolbar.panels.sql import SQLDebugPanel
@@ -214,6 +216,19 @@ def test_recording(self):
# ensure the stacktrace is populated
self.assertTrue(len(query[1]['stacktrace']) > 0)
+ @unittest.skipUnless(connection.vendor=='postgresql',
+ 'Test valid only on PostgreSQL')
+ def test_erroneous_query(self):
+ """
+ Test that an error in the query isn't swallowed by the middleware.
+ """
+ from django.db import connection
+ from django.db.utils import DatabaseError
+ try:
+ connection.cursor().execute("erroneous query")
+ except DatabaseError as e:
+ self.assertTrue('erroneous query' in str(e))
+
def test_disable_stacktraces(self):
panel = self.toolbar.get_panel(SQLDebugPanel)
self.assertEquals(len(panel._queries), 0)
Something went wrong with that request. Please try again.