Opened 18 years ago

Closed 15 years ago

Last modified 6 years ago

#303 enhancement closed wontfix (wontfix)

[patch] Persistent transactions for adbapi

Reported by: grib Owned by: Dave Peticolas
Priority: normal Milestone:
Component: core Keywords:
Cc: Dave Peticolas, itamarst, jknight, grib Branch:
Author:

Description


Attachments (3)

diff.out (4.5 KB) - added by grib 18 years ago.
adbapi.diff (4.5 KB) - added by grib 18 years ago.
adbapi.2.diff (5.0 KB) - added by grib 18 years ago.

Download all attachments as: .zip

Change History (23)

Changed 18 years ago by grib

Attachment: diff.out added

comment:1 Changed 18 years ago by grib

This patch adds a new API to ADBAPI for cases when database
Transactions need to outlive a single interaction.  The use
case that prompted it is a Woven app I am converting to
adbapi.  In it, SQL commit/rollback semantics are used to
ensure that a batch of edits are published together.  An
open SQL transaction can persist across many HTTP requests,
for example. 

The changes are pretty straightforward, with the exception
of the tricky interactions between the threadpool and the
connections used by the ADBAPI ConnectionPool.  Given the
limits of the threadpool API, I think I made just about the
minimal changes, but it looks a little kludgy to me (the bit
with the Semaphore particularly).

Whaddaya think?

comment:2 Changed 18 years ago by jknight

Nice, I was going to write that; now I don't have to!

comment:3 Changed 18 years ago by grib

btw that patch was more like a sketch (worked to initial tests but incomplete).
 I have a better patch with testcases coming up, just wanted to see if the idea
would float or sink.

comment:4 Changed 18 years ago by itamarst

Hello Mr. Enterprise Maintainer!

Changed 18 years ago by grib

Attachment: adbapi.diff added

comment:5 Changed 18 years ago by grib

Here's an updated patch (which works, unlike the last one).

dave_p, you there? 

b.g.

comment:6 Changed 18 years ago by Dave Peticolas

Hey Bill, I'm here. I was away from my computer and couldn't
remember my password. This sounds like a cool patch, I'll get
back to you this weekend.

comment:7 Changed 18 years ago by Dave Peticolas

A couple of comments:

1. The two new _run*InTransaction define a new variable 'conn'
   as the _connection member of the transaction but then don't
   appear to use that variable later.

2. Why do those same two functions call trans.reopen()? Won't
   that commit or rollback whatever query you submitted previously?

3. Could the startTransaction docs mention the need to use the
   transaction with the run*InTransaction methods to avoid blocking?
   Or am I reading that wrong?

4. I think _startTransaction should be using reactor.callFromThread,
   instead of reactor.callInThread so that the transaction will be
   returned by the main loop.

comment:8 Changed 18 years ago by grib

Hi dave.

Re your comments: 

1. [AGREE] Those are edit-turds.  Sorry. 

2. [DISAGREE] trans.reopen just closes the DBAPI-level cursor object and starts
a new one.  You don't necessarily need a fresh cursor for each query, but it
doesn't hurt.  You can close and open cursors as often as you want. The only
thing to remember is that cursors get invalidated when the transaction they were
created within is committed or rolled-back, so it's best not to count on them
hanging around unless you're real careful.

3. [AGREE] It should mention that you need to commitTransaction or
rollbackTransaction to prevent tying up a thread, for sure.   In general, if
you're going to keep transactions open for decent periods of time, you need to
bump up the cp_max.

4. [DUNNO] If you say so :) 

Also:  My patch accidentally deleted line 239 from adbapi.py, which breaks usage
of the non-Transactified runQuery()... that's the very last line of the patch. 
Ignore that line :)

comment:9 Changed 18 years ago by grib

There are still some big problems with this approach.  It's very easy to
auto-DOS yourself if you issue more startTransaction calls than cp_max.  I need
to think about how to address this.  Don't commit the current patch :( 

b.g.

comment:10 Changed 18 years ago by Dave Peticolas

I wasn't planning on committing until the unit tests were ready.

As far as the DOS problem goes, is there really any way to avoid that?
It seems like a built-in consequence of any solution to the problem you
are trying to solve, i.e., if you can reserve transactions for long-term
use, you can reserve too many, too :)

Re my comment #4, I'm pretty sure that callInThread and callFromThread
are paired, with the latter being used to bring out the results of whatever
you started with callInThread. Itamar or someone else will know better, though.

comment:11 Changed 18 years ago by grib

The DOS problem is avoidable; I have a fix for it.  The problem wasn't that you
could block, it was that you could actually deadlock with no way out... all the
threads tied up in startTransaction and no threads to actually run SQL on those
transactions, including commit() or rollback().  

I've fixed that in my tree by calling the run*InTransaction functions,
commitTransaction, and rollbackTransaction from a separate pool of threads which
have no db connections.

About #4, there's something fishy going on.  My current demo/example app works
if it's callInThread but hangs if callFromThread.  However there are probably
other things going on.  I'll get back to you on that. 

b.g.

comment:12 Changed 18 years ago by Dave Peticolas

Ok, I see what you're saying. And now that I understand it better,
instead of adding another threadpool, you should probably just use
the reactor's threadpool for simplicity. The threadpool in adbapi
is really just for managing the db connections.

comment:13 Changed 18 years ago by Dave Peticolas

Or, hmm. Maybe not. What determines how many transactions
you can have open at once? The size of the new pool?

Changed 18 years ago by grib

Attachment: adbapi.2.diff added

comment:14 Changed 18 years ago by grib

The number of db connections is determined by the size of the threadpool; each
thread gets one connection. Transactions are just like any other database
operation, so right now the max number of transactions is cp_max, the max db
connections. 

The threadpool uses next-available scheduling and a single work queue, so when
you submit work to the threadpool you can't know which thread (therefore which
connection) will do the work.  That's why the transaction stuff has to block the
thread that opens the transaction -- to keep it out of the normal work rotation. 

Using the reactor's threadpool won't work with the current threadpool
implementation... there's no way to "reserve" threads to run queries in open
transactions, and even if you could, you'd be wasting available connections
(with cp_max=2, for example, if you opened a transaction, you'd have to reserve
the other thread to run queries in the transaction, which would cause any other
use of the connectionpool to block even though there's a completely unused
connection).

A way to do it without an extra threadpool would be to make per-thread work
queues for threads with open transactions, but <shrug> my current patch
(attached) is IMO simpler than putting all that in place, and doesn't change any
of the user-visible semantics of what cp_min/cp_max mean. 

b.g.

comment:15 Changed 18 years ago by grib

...but of course there are still problems.  With a pool of threads to run
queries in open transactions, you can get multiple queries on one connection
without proper ordering.  Confusion ensues. 

Still chewing on it,
b.g.

comment:16 Changed 18 years ago by Dave Peticolas

Maybe you could uses Queues instead of Semaphores?
And instead of having a separate threadpool for doing
the work, you could just submit queries to the right queue?

comment:17 Changed 18 years ago by grib

yeah, that's what i've been working on today. I'm pretty close (works most of
the time :)

b.g.

comment:18 Changed 15 years ago by Dave Peticolas

Component: conch
Resolution: wontfix
Status: newclosed

I don't think this patch ever reached a totally working state, and as far as I know, grib is not in any position to move it there. Persistent transactions might be useful, but also quite complex, so starting over is probably for the best.

comment:19 Changed 14 years ago by Jean-Paul Calderone

Component: conchcore

comment:20 Changed 6 years ago by GitHub <noreply@…>

In ec71728:

Error: Processor CommitTicketReference failed
 does not appear to be a Git repository. See the log for more information.
Note: See TracTickets for help on using tickets.