Re: pysqlite design decisions

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

First, thanks for your constructive criticism.

glyph-TyWPi3/[email protected] wrote:
> [...]
> 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...?).

There was no chance to fix it in earlier SQLite releases, now that SQLite
exposes the autocommit mode
(http://sqlite.org/capi3ref.html#sqlite3_get_autocommit), it could be fixed
 for newer SQLite versions.

> 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.

I didn't think in terms of a transaction management layer so far, mostly
because pysqlite doesn't do anything *so* fancy IMO, only what is required
anyway by the DB-API. Later on, the possibility to switch the

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

As required by the DB-API.

> 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". 

SQLite itself did not use a term for its various locking strategies
(DEFERRED/IMMEDIATE/EXCLUSIVE) that can be controlled via the BEGIN
statement. So I borrowed the term from other databases "isolation level"
that can also controlled via a parameter of the same name in psycopg.

> As far as I can tell it's impossible to exercise any control over the 
> statement cache from Python.

That's true. The statement cache is a different beast and only got
introduced later in version 2.1. In fact, I thought about exposing
"prepared statement" objects before, but my guess was that the builtin
cache from pysqlite is good enough for all practical purposes. If there is
a demand to offer that feature, I think it shouldn't be *that* hard.

> 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.

Have you benchmarked Axiom with pysqlite 2.0.x and a newer version like
2.3.x? My guess would be that Axiom adds so much overhead anyway that using
prepared statements doesn't make much of a difference anyway. I just want
to make sure we talk about real life problems here.

> 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.

Ok, I understand why it would make sense for something like Axiom to not
have two different cache strategies for objects and SQL statements.

> (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 don't fear adding additional features on top of the DB-API, but I want to
be as compatible with the DB-API and with previour pysqlite releases as
possible.

> 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".

And that sounds like a reasonable policy to me :-)

> 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;

Ok, I see the problem. Unfortunately we need to have pysqlite open
transactions for you, because SQLite doesn't do it for us and the Python
DB-API requires us to do so.

> 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.

Ouch.

> [...]
> 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.)

Sorry for the trouble - I'm still waiting for the initd.org admins to fix
the webserver problems.

> 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.

It took me a while to understand this. I don't think triggers are a problem
here - there are no triggers that can be, well, triggerred by SELECT. But
it is possible in theory to execute DML or DDL in a SQL function. Of course
that's perverse, considering that a function should have no side effects.

> 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. 

Again, I don't think pysqlite is to blame, but if anything, the DB-API with
its rather strange mixing of concepts of connection and cursors. There
should really have been a separate transaction object in the DB-API, and
cursors are mostly superfluous. They should have been the result of
execute() statements, instead of being created beforehand and containing
the result set, i. e.:

con = sqlite.connect(...)
trans = con.transaction()
trans.execute("insert ...")
cur = trans.execute("select ...")
for row in cur:
  ...
trans.commit()

But I'm disgressing ...

> [...]
> 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 know of this problem. That's why I have once started with implementing a
logging layer for pysqlite:
http://initd.org/tracker/pysqlite/browser/sandbox/preliminary_logging_patch.dif

Unfortunately, that proved to be rather hard to do in C - I know what's
coming now: so don't do it in C ...

> 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.

You're right, I wouldn't like that. Not because the everything-in-C is a
dogma, but because I don't want a grand rewrite. I do think, however, that
most of what you like to see can be implemented in a more iterative
approach and without big changes.

> I would like to understand the motivation behind the way the 
> transactions currently work; why the implicit commits?

As said above, required by the DB-API (the text and the interpretation on
the DB-SIG mailing list).

> What about a
> "CREATE TABLE" statement requires that the database should be committed?

That was really only because in my tests, some DDL commands could safely be
executed in transactions, while SQLite failed for others (maybe now all DDL
commands are transaction safe, I don't know).

> 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.

+1

>  * 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.

+0 looks like a lot of effort - if you want to help and we do this in an
experimental branch in SVN, I'm in.

>  * wrap statement compilation and parameter binding for Python

+1

>  * Expose the bottom layer to Python applications (perhaps via a
> different module name?) so that it is easier to get "raw" access to sqlite.

- -0. This is what APSW offers today. It feels unproductive to duplicate all
their efforts. I wouldn't object to just distribute APSW as a submodule
with pysqlite though.

>  * 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

- -1 on the rename because I don't want any backwards-incompatible changes.
If there really will be a separate layer, it needs to be exposed somehow,
of course.

> 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. 

+1

> 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.

If it doesn't become clear after reading the DB-API 2.0 spec, drop me a
note and I will dig in the DB-SIG archives to find relevant posts.

Cheers,

- -- Gerhard
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.3 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFFcwv1dIO4ozGCH14RAtJAAKCCiLMgokW/oqLS4f7P82vaizVhkACdHid4
eHZUu9yiMzLsRFwsu3abIQ8=
=W0mt
-----END PGP SIGNATURE-----