Re: pysqlite design decisions

(Apologies if this doesn't show up in the same thread for you - I didn't subscribe to the list in time to send a real "reply")

For those of you who don't know, I'm the primary author of, among other things, Axiom ( http://divmod.org/trac/wiki/DivmodAxiom ), an SQLite-based object database.

I suggested ( http://initd.org/tracker/pysqlite/ticket/185#comment:1 ) that PySQLite should really have a 2-layer architecture, with the lower (API interface) layer being in C, and the upper (transaction management) layer being in Python.

My critique of the current code may seem rather harsh, so please keep in mind that I've been successfully using PySQLite for more than a year now, and I really appreciate the work that has gone into it so far.  If I seem mean, it is only because I have a passion for excellence in open source software and I would like to see PySQLite get even better.

Now that that's out of the way, "flame on!" :)

There are actually 3, not 2, layers to pysqlite that I want to separate:

 1. the SQLite access API
 2. the transaction management layer
 3. the statement cache

It's clear, to me at least, that the higher levels here are inadequately tested.  In some cases, if you use them, they will completely break features of SQLite itself (for example, ticket #97 - which apparently won't ever be fixed...?).  I should have spoken up about this sooner, because #170/#185 was clearly obvious to Axiom's test suite as of the 2.2 release.

If there is a real need to optimize the transaction management layer into C then that makes perfect sense, but that should be done *after* a tested version of this has been developed and shaken out in Python.

Access to these features is currently ad-hoc and implicit.

Axiom has a huge amount of information about when statements are used and re-used, and when transactions should be committed.  This means that we want to use the bottom-most layer of pysqlite.  The API for doing this is highly misleading: "isolation_level=None" does not actually mean "provide a lower isolation level", it means "use SQLite's inherent isolation / concurrency primitives rather than those in PySQLite".  As far as I can tell it's impossible to exercise any control over the statement cache from Python.

If it were possible for Axiom provide its own statement cache, I am fairly sure it would be considerably more efficient than the built-in cache, because rather than keeping an in-memory dictionary around and looking up strings in it, we could simply associate compiled statement objects with query objects.  Right now, we just hang on to the generated string, in the hopes that it will be found in pysqlite's internal dictionary.  This is spooky action-at-a-distance programming, where what I want to do is be guaranteed a hit on the statement cache, because I know exactly which statement I'm referring to, and how long it needs to live for the user's code to be efficient.  I already have an object to attach it to.

Splitting this into a multi-level architecture and exposing each level to Python would allow us to use the statement preparation / compilation API in the same way that the caching wrapper would, even if there were both a C and Python implementation of said wrapper.

(Just to be clear: I blame this particular misfeature more on the lack of a PreparedStatement analogue in DB-API standard than on PySQLite.  I can understand if there's no desire to provide a separate, publicly supported statement-manipulation API because that's not part of the Python DB access spec... I'd still like it if it were at least Python-visible.  I don't mind putting a bit of extra effort in to track and support multiple releases, as long as it's possible.)

Currently (as bug #185 indicated) there are lots of possibilities for bugs in the transaction management layer which are difficult to avoid if all you need is a simple interface to pysqlite.  Users may want a different transaction policy from what pysqlite provides, but the options are "use what's built in" or "write a completely separate layer on top".

I think that pluggability at this level is important, partially because I think that the current mechanism for determining when to commit is completely broken :).  Someone familiar with SQL databases, even familiar with using SQLite from other programming languages, can easily write (the Python equivalent of) a transaction like

  INSERT INTO app_internal_schema_features VALUES('foo');
  CREATE TABLE foo (bar INTEGER);
  INSERT INTO foo VALUES (1);
  COMMIT;

and unless they've read http://www.initd.org/pub/software/pysqlite/doc/usage-guide.html#controlling-transactions
they will assume that this is going to work the same way it does in every other database - which is to say you won't have a 'foo' table unless the appropriate corresponding entry has been added to app_internal_schema_features.

Worse, there's no special API or error indicating that you need to be aware of this particular feature; you might only find out about pysqlite's transaction-control idiosyncracies the first time that your database becomes corrupted.  Luckily I discovered this when a database became corrupted in a test run, not in production - and isolation_level=None does at least allow one to fix the potential breakage.

I mentioned the possibility of bugs.  While writing this email I discovered another bug: pysqlite statement detection does not integrate with the sqlite parser, it has its own way of determining what type of a statement is being executed (string prefixes).  This means that there is always the possibility that its idea of what's going on will diverge from the actual database engine's.  It takes care of whitespace prefixes, but let's say you were loading your SQL statements from resource files of some kind and you had a statement like this:

    -- OK now let's go look up those invoice line items
    SELECT * FROM invoice_line_item WHERE ...;

The comment will cause the statement to be identified by PySQLite as a "STATEMENT_OTHER" and silently commit any pending transaction - again, no way for the developer to notice this.  (I would have submitted this as a bug in trac, but it's giving me an "Internal server error" on login now.)

I believe there is also a bug with "executescript" being detected as the statement type of its _first_ statement - unless it begins with a comment.

While this bug could be fixed, this problem becomes worse when triggers and functions come into the mix.  It's not even *possible* to tell what sort of statement is going to be run just by looking at the syntax.

You might just say "well, don't use triggers, then", just as the current transaction-control page advises against using "on conflict rollback" but it is possible to use SQLite in applications from more than one programming language, and in those situations triggers are used rather heavily, to make sure that the different languages are using the database in the same way.  Pysqlite could be issuing the same statements as tcl or ruby, but behaving completely differently.  It also seems obvious to me that telling people not to use features of a library you're wrapping in a wrapper is poor form.

Even if the developer is very concerned with transaction semantics and watching a "debug log" of all executed SQL, they won't catch it.  In addition to sometimes silently executing its own statements on your behalf, the PySQLite wrapper provides no debugging API of its own; most libraries make the (otherwise reasonable assumption) that printing everything they pass to "execute" is sufficient.

I would definitely put at least a little time into writing patches, if I felt that the direction I was hoping for was supported.  Unfortunately, right now my feelings are that all transaction control should simply be removed from pysqlite, and that the statement cache should be moved into Python.  I don't think patches to do this would currently be accepted.

I would like to understand the motivation behind the way the transactions currently work; why the implicit commits?  What about a "CREATE TABLE" statement requires that the database should be committed?

I'm not going to just offer a heap of what's wrong with no suggestions for how to make it better though, so here's a plan, which we can perhaps come to some agreement on and flesh out:

 * add a "debug" API which allows you to re-direct the text of all executed SQL statements if enabled - and provide appropriate comments when BEGIN and COMMIT are executed by the engine.
 * Re-factor the existing C layer into multiple objects, and expose them to Python so that the transaction controller and statement cache can be tested independently of actually running SQL.
 * wrap statement compilation and parameter binding for Python
 * Expose the bottom layer to Python applications (perhaps via a different module name?) so that it is easier to get "raw" access to sqlite.
 * Change the rather vague "isolation_level" parameter to a "transaction_policy" parameter, so that the layers can be composed more easily, and new policies can be prototyped and profiled in Python before C optimization

I also feel strongly right now that the default transaction policy ought to be changed so that DDL statements which happen in the _middle_ of a transaction cause an error.  At the very least, you ought to force the user to explicitly commit before an effective autocommit transaction takes place.  I'd really like to hear more about the design rationale behind the way it works now before I'd say I'm sure about that though.

_______________________________________________
pysqlite mailing list
pysqlite-IAPFreCvJWPBWskQ1e/[email protected]
http://lists.initd.org/mailman/listinfo/pysqlite