Re: pysqlite design decisions

On 3 Dec, 05:40 pm, [email protected] wrote:
>-----BEGIN PGP SIGNED MESSAGE-----
>Hash: SHA1
>
>First, thanks for your constructive criticism.

You're welcome!  Thank *you* for handling it with such grace and aplomb :).  I think this has been a very productive exchange.

>glyph-TyWPi3/[email protected] wrote:

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

OK, that's good.  (I'd comment on the ticket if I could log in...)

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

Aaahhhh.  OK.  This makes a lot more sense to me now.  (Snipping all other explanations of this later.)  I didn't entirely understand that part of the DB-API spec.

>> Access to these features is currently ad-hoc and implicit.
>
>As required by the DB-API.

DB-API _does_ provide a ridiculous amount of flexibility in the parameters to the "connect" method though :).

>> Axiom has a huge amount of information (...)

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

I still think the term is slightly misleading, but I can appreciate the history and don't really think it was a big deal.  I was naming everything I could think of that was wrong in this message without really assigning priority to them, and this is a minor issue.

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

Just a thought: one way to do this would be to simply have a (cursor/connection).prepare() method, which returns an object that can be used interchangeably with a string passed to "execute".  This would allow the C execute to be split up into smaller functions as well, since it could simply check if the argument were a string, and call "prepare" itself, if not.

>> If it were possible for Axiom provide its own statement cache, I am
>> fairly sure it would be considerably more efficient 

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

You're correct.  I actually have profiled Axiom and I know where it's spending 99% of its time - unsurprisingly, not in the statement cache code :).  As with the naming of "isolation_level", this is a minor issue for me.

The prepare/execute distinction that I mentioned above is really the feature I really want here - architectural clarity and smaller functions, not speed.

>> Splitting this into a multi-level architecture and exposing each level
>> to Python [...]

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

Right now it's just treating "strings" and "statement objects" as the same.  The main thing I actually want from a prepared-statement type is avoiding accidental use of "statement" objects as strings.

>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 completely agree.  There is no reason to break backwards compat or DB-API conformance while dealing with any of these issues.

>> 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 :-)

All right - as long as it's easy and it works :-)

>> [snip description of schema-modifying transactions]

>> you won't have a 'foo' table
>> unless the appropriate corresponding entry has been added to
>> app_internal_schema_features.
>
>Ouch.

Yeah.  Axiom maintains just such a parallel schema table (with higher-level information about how Python objects have been mapped into the database) and this is _the_ bug in pysqlite which has cost me the most time.

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

No worries, I know how this goes.  Twisted has had some really major issues lately trying to keep our trac up and running.

Report that bug when you get a chance though, if you don't mind.

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

I certainly haven't done anything like this, but I can imagine situations where it might make sense; for example if the function were doing some on-demand caching.  I wouldn't even *really* object if this just didn't work; it's the not following documented behavior that bothers me.

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

Wellll, I don't know about *that* :)

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

Yeah, DB-API is not great.  It encourages sloppiness at many levels, this just being one of them.  I completely agree though.  Separate transaction objects, separate 

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

Still, that API is a very nice idea.  One of the scariest features of sqlite (not pysqlite) is that leaving a cursor from a SELECT open with unclaimed results will effectively delay a COMMIT forever... such a transaction object would be a nice place to check for that.

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

Huh, that looks a lot more complicated and specific than what I want.  I would just like to be able to specify a python callable that gets called with a 2-tuple of (sql, args) each time a statement was executed.  (or really, (statement, args), if there were statement objects).

>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 understand that the core feature here does need to be done in C, but the fancy line-number printing and stuff could be done in Python.  Maybe some time next week I'll have a few minutes to look at writing a patch.

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

Oh, I _totally_ agree.  I'm coming off of 3 "grand rewrites" this year.  *Never again*.  I am actually going to make that a part of Twisted commit policy soon.

>I do think, however, that
>most of what you like to see can be implemented in a more iterative
>approach and without big changes.

Great.

>> 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've been running lots and lots of DDL statements in transactions and testing the failure cases extensively.  As far as I can tell, if this was ever broken in sqlite, it's fixed now.

Do we agree, then, that one possible next step is just to eliminate all the implicit commits, and leave sqlite in non-autocommit mode from now on?

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

OK

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

I don't know that this is really pressing for me either.  I was just outlining things that maybe one day I'd like to do, trying to see what we'd agree on.  Maybe in 18 months I will write a patch to do this fully :).  You might see something from me that splits up some functions into smaller pieces to move in that direction though.

>>  * wrap statement compilation and parameter binding for Python
>
>+1

Great.

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

This is not a -0, this is a +10000!!!!  :-D  I would *LOVE* to see APSW and pysqlite merge, even at the very basic level of just being in the same repository.  This would eliminate confusion for people wondering which binding to download, and could eventually lead to a big reduction in duplicated effort.  From my point of view this would be the _best possible_ way to expose the lower layer, get all bug reports going to the same place, etc etc.

While I am definitely against Grand Rewrites, I do not think it would be bad if the duplicate code between the projects were gradually unified over the next few years.

>>  * 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 did not mean to say that isolation_level should be removed, since there is no point to breaking backwards compatibility.

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

This is definitely the biggest flaw in my opinion, so I'm very happy to hear that you agree.

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