Edgewall Software

Changes between Initial Version and Version 2 of Ticket #8443


Ignore:
Timestamp:
Jul 2, 2009, 10:32:48 PM (15 years ago)
Author:
Remy Blank
Comment:

Indeed ;-)

Legend:

Unmodified
Added
Removed
Modified

  • Ticket #8443 – Description

    initial v2  
    55The database connection pool in trac is called with every env.get_db_cnx call.  It has a list of "active" connections, which are connections currently in use, and then the pool.  The typical pattern that happens when using a database connection looks like:
    66
    7 - some function in trac is entered
    8 - get connection
    9 - if connection in active list for thread, use that
    10 - otherwise get one from pool (or created one)
    11 - a cursor is created
    12 - psycopg may execute a BEGIN statement, based on the default settings for the postgres server.  The installed default for postgres causes psycopg to do that.
    13 - function in trac finishes
    14 - connection wrapper is garbage collected, causing the wrapper to "reinsert" the connection into the pool
    15 - if it is the last "reference" to the connection, trac attempts a rollback (which typically matches the begin above)
    16 - if it is NOT the last "reference" to the connection, and no explicit rollback was done (eg. after an insert, trac/plugins usually do a commit) then the pool also does not rollback (THIS IS THE PROBLEM).  The result is that the begin statement above remains in effect, causing "IDLE in transaction".
     7 - some function in trac is entered
     8 - get connection
     9 - if connection in active list for thread, use that
     10 - otherwise get one from pool (or created one)
     11 - a cursor is created
     12 - psycopg may execute a BEGIN statement, based on the default settings for the postgres server.  The installed default for postgres causes psycopg to do that.
     13 - function in trac finishes
     14 - connection wrapper is garbage collected, causing the wrapper to "reinsert" the connection into the pool
     15 - if it is the last "reference" to the connection, trac attempts a rollback (which typically matches the begin above)
     16 - if it is NOT the last "reference" to the connection, and no explicit rollback was done (eg. after an insert, trac/plugins usually do a commit) then the pool also does not rollback (THIS IS THE PROBLEM).  The result is that the begin statement above remains in effect, causing "IDLE in transaction".
    1717
    1818Fixing this in the pool would be the wrong place since you can never know if a rollback is safe on an "active" connection.  I'm also wondering if the rollback in there currently is due to bugs reported on this issue before.
    … …  
    2121
    2222Without requiring a repro, this illustrates how it happens:
    23 
     23{{{
     24#!python
    2425def do_something(env):
    2526    db = env.get_db_cnx()
    … …  
    3738    # once we're done, db is reinserted
    3839    # to the pool and rollback is called
    39 
     40}}}
    4041With the above outline, two BEGIN statements were executed, but only on rollback.
    4142
    4243The simple hack fix is to modify main.dispatch_request to get a handle on the database connection prior to handling the request, then doing a rollback after the request:
    43 
     44{{{
     45#!python
    4446    req = Request(environ, start_response)
    4547    try:
    … …  
    5052        if env and not run_once:
    5153            env.shutdown(threading._get_ident())
    52 
     54}}}
    5355I call it a hack fix because it seems ugly to me, but it's the only place that this can be handled reliably without the potential to step on a commit that is in process, and without adding commit/rollback all over the trac (and plugins) code. 
    5456
    5557The fix does not break current transaction area's (eg. after an update/insert) since psycopg will issue a new BEGIN on the next execute if commit or rollback was called.  Updates, Inserts, etc. should remain explicit about calling commit, since a select will not get the new data unless that is done.
    5658
    57 It also seems like some extra work is happening in the current db pooling, such as creating a PooledConnection class for each call to env.get_db_cnx even if the connection is in the "active" list.
     59It also seems like some extra work is happening in the current db pooling, such as creating a `PooledConnection` class for each call to `env.get_db_cnx` even if the connection is in the "active" list.
    5860
    5961Looking at other bugs regarding this issue, it seems that is would also be a good idea to force close the connection at the end of a request IF the status is "in transaction" still.  Log or raise and error with it as well.  This would at least prevent problems around replication and vacuuming, and let people know there is a bug (likely a missing commit in some plugin).
    60 
    61 
    62 
    63