On Tuesday 11 December 2012, Ian Kelly wrote:
> On Tue, Dec 11, 2012 at 6:31 AM, Shai Berger <s...@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.
--
You received this message because you are subscribed to the Google Groups
"Django developers" group.
To post to this group, send email to django-developers@googlegroups.com.
To unsubscribe from this group, send email to
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/django-developers?hl=en.
