Oracle backend bug makes testing on oracle slower?

60 views
Skip to first unread message

Shai Berger

unread,
Dec 11, 2012, 8:31:53 AM12/11/12
to django-d...@googlegroups.com
Hi all,

I've just been hit by a very simple and annoying bug on Oracle, on Django
1.3.4. Without testing, it seems to no longer be as bad on master, but a shade
of it still remains.

The bug is this: The DatabaseFeatures for the Oracle backend do not specify
that it supports transactions.

On 1.3.4, this means the value is only set if confirm() is called, and when it
is, it only affects a single connection; it does not appear to be called
whenever a connection is created -- while database test creation calls it, by
the time the tests use it it is all forgotten and testing is relegated to the
old behavior of flush for every test.

On master, where the generic supports_transactions is a cached property, this
means that for every connection opened, 4 statements are being executed to
check if the database (still) supports transactions.

1. Oracle supports transactions. is there a reason not to just add the line

supports_transactions = True

to its DatabaseFeatures class?

2. Database features, typically, do not change between opening of connections
within a single server run. Shouldn't we consider saving these features --
even the dynamic ones -- on the DatabaseFeatures class, instead of its
instances?

Thanks,
Shai.

Ian Kelly

unread,
Dec 11, 2012, 1:32:02 PM12/11/12
to django-d...@googlegroups.com
On Tue, Dec 11, 2012 at 6:31 AM, Shai Berger <sh...@platonix.com> wrote:
> Hi all,
>
> I've just been hit by a very simple and annoying bug on Oracle, on Django
> 1.3.4. Without testing, it seems to no longer be as bad on master, but a shade
> of it still remains.
>
> The bug is this: The DatabaseFeatures for the Oracle backend do not specify
> that it supports transactions.
>
> On 1.3.4, this means the value is only set if confirm() is called, and when it
> is, it only affects a single connection; it does not appear to be called
> whenever a connection is created -- while database test creation calls it, by
> the time the tests use it it is all forgotten and testing is relegated to the
> old behavior of flush for every test.
>
> On master, where the generic supports_transactions is a cached property, this
> means that for every connection opened, 4 statements are being executed to
> check if the database (still) supports transactions.

While connections come and go, the DatabaseWrapper objects are stored
in the ConnectionHandler on a per-thread basis and should be reused
for subsequent connections. So these tests *should* run only once per
configured connection per thread. Can you demonstrate that they are
actually being run on every connection?

> 1. Oracle supports transactions. is there a reason not to just add the line
>
> supports_transactions = True
>
> to its DatabaseFeatures class?

I see no harm in it, although per the above I don't think it will help
as much as you hope. Since the test involves a CREATE TABLE though,
and since DDL is slow in Oracle, and since there's really no guarantee
that the user even has the necessary permissions to create the table
in the first place, I think it's a good idea to add this in any case.

> 2. Database features, typically, do not change between opening of connections
> within a single server run. Shouldn't we consider saving these features --
> even the dynamic ones -- on the DatabaseFeatures class, instead of its
> instances?

This would lead to bugs in multi-DB setups where the user has
configured connections to multiple databases of the same type but
different versions (and different feature sets). I don't know how
common that is, but it is better to be correct and slow than fast and
buggy.

Shai Berger

unread,
Dec 11, 2012, 2:53:55 PM12/11/12
to django-d...@googlegroups.com
On Tuesday 11 December 2012, Ian Kelly wrote:
> On Tue, Dec 11, 2012 at 6:31 AM, Shai Berger <sh...@platonix.com> wrote:
> > Hi all,
> >
> > I've just been hit by a very simple and annoying bug on Oracle, on Django
> > 1.3.4. Without testing, it seems to no longer be as bad on master, but a
> > shade of it still remains.
> >
> > The bug is this: The DatabaseFeatures for the Oracle backend do not
> > specify that it supports transactions.
> >
> > On 1.3.4, this means the value is only set if confirm() is called, and
> > when it is, it only affects a single connection; it does not appear to
> > be called whenever a connection is created -- while database test
> > creation calls it, by the time the tests use it it is all forgotten and
> > testing is relegated to the old behavior of flush for every test.
> >
> > On master, where the generic supports_transactions is a cached property,
> > this means that for every connection opened, 4 statements are being
> > executed to check if the database (still) supports transactions.
>
> While connections come and go, the DatabaseWrapper objects are stored
> in the ConnectionHandler on a per-thread basis and should be reused
> for subsequent connections. So these tests *should* run only once per
> configured connection per thread. Can you demonstrate that they are
> actually being run on every connection?
>

As I noted above, I haven't actually tested on master, so I can't be sure;
this was just my reading of the code. As far as I'm aware, though, the basic
management of database wrappers and connections hasn't changed much since 1.3,
and I can demonstrate that on 1.3.4, a new connection uses a new
DatabaseFeatures object. I found this issue not by optimizing for speed, but
by using a test-runner which installs some objects in the DB before running
the test-suite; to my surprise, the objects weren't available, because the
test framework, thinking the backend didn't support transactions, flushed the
DB (as mentioned, on 1.3.4 the setting of supports_transactions takes a call
to confirm(), but one is made, and running tests is single-threaded).

> > 1. Oracle supports transactions. is there a reason not to just add the
> > line
> >
> > supports_transactions = True
> >
> > to its DatabaseFeatures class?
>
> I see no harm in it, although per the above I don't think it will help
> as much as you hope. Since the test involves a CREATE TABLE though,
> and since DDL is slow in Oracle, and since there's really no guarantee
> that the user even has the necessary permissions to create the table
> in the first place, I think it's a good idea to add this in any case.
>
Should I open a ticket for it? It is a one-line patch...

> > 2. Database features, typically, do not change between opening of
> > connections within a single server run. Shouldn't we consider saving
> > these features -- even the dynamic ones -- on the DatabaseFeatures
> > class, instead of its instances?
>
> This would lead to bugs in multi-DB setups where the user has
> configured connections to multiple databases of the same type but
> different versions (and different feature sets). I don't know how
> common that is, but it is better to be correct and slow than fast and
> buggy.

While I doubt that such setups are common enough to justify a price paid by
the whole community, I can accept this answer as "we considered it and decided
against". But then, I would ask for backends to rely as little as possible on
dynamic features. A user can easily monkey-patch sense into the backend
DatabaseFeatures, but that's no replacement for sensible defaults.

Thanks,
Shai.

Florian Apolloner

unread,
Dec 11, 2012, 3:15:03 PM12/11/12
to django-d...@googlegroups.com


On Tuesday, December 11, 2012 8:53:55 PM UTC+1, Shai Berger wrote:
Should I open a ticket for it? It is a one-line patch...

Please try to test this on master first, we most likely won't patch 1.3.

Cheers,
Florian

Shai Berger

unread,
Dec 11, 2012, 3:36:44 PM12/11/12
to django-d...@googlegroups.com
This was referring only to the setting of supports_transactions on Oracle,
which Ian appeared to support. I had no illusions that it would be ported to
1.3 or even 1.4 (and I have an easy workaround for these versions anyway).

Thanks,
Shai.

ajayka...@gmail.com

unread,
Dec 12, 2012, 3:45:05 AM12/12/12
to sh...@platonix.com, django-d...@googlegroups.com
----------
Sent via Nokia Email
--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-d...@googlegroups.com.
To unsubscribe from this group, send email to django-develop...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.


Reply all
Reply to author
Forward
0 new messages