postgresql_psycopg2 never restores autocommit mode when leaving transaction management

[Brodie Rao]

When using the postgresql_psycopg2 backend with DB-level autocommit mode enabled, after entering transaction management and then leaving it, the isolation level on the psycopg2 connection is never set back to autocommit mode.

And to make matters worse, the isolation level on the DatabaseWrapper object stays set to READ COMMITTED, which means all new database connections made in the same process won't be using the AUTOCOMMIT isolation level.

This is particularly problematic with pgbouncer, which will essentially stop acting like a connection pool if you disconnect from the server mid-transaction. When using psycopg2 with the default READ COMMITTED isolation level, the database driver will implicitly open a transaction for each new connection. If the last query in the connection isn't .save() or .update(), Django won't have issued COMMIT or ROLLBACK. If Django closes the connection without cleaning up the transaction, pgbouncer can't safely reuse that connection. Thus it's forced to close the server connection, and you get an error stating "closing connection: unclean server".

Turning on autocommit mode and explicitly using transactions when they're needed is one way of mitigating that problem, but this bug prevents that workaround from being viable. I think the issue with not cleaning up implicitly opened transactions is probably worth a separate bug report, which I'd be happy to create if my description makes sense to others.

I'm attaching three patches to this ticket related to the original autocommit mode problem described above. The first patch adds two tests to modeltests.transactions.tests that confirm autocommit mode is initialized correctly and that it's restored correctly after leaving transaction management. The second patch corrects the DB feature tests for transaction support when using psycopg2's autocommit mode.

The third patch fixes the bug by correcting what seems to me to be a nonsensical conditional in DatabaseWrapper._leave_transaction_management(). Specifically, it only sets the isolation level back to autocommit mode if three conditions are satisfied:

1. We've asked for DB-level autocommit mode to be enabled.

2. We're not in managed mode (i.e., someone has called transaction.managed(False)).

3. The isolation level isn't already set to AUTOCOMMIT.

The second check is what prevents the isolation level from ever being restored; neither the transaction decorators nor the transaction middleware ever call transaction.managed(False) before calling transaction.leave_transaction_management(). It's also likely that other people writing custom transaction decorators don't do this either, so it seems reasonable to remove the check. I can't think of any reason you'd want to keep the isolation level at READ COMMITTED when you've asked to leave transaction management and you've also asked for DB-level autocommit mode.

Also, I believe this bug has been present in the psycopg2 wrapper since autocommit mode was introduced. I also believe the second bug I mentioned has been present for many releases.

[Brodie Rao]

Also, I should mention that I've run the test suite with my patches applied against the releases/1.3.X branch. All tests pass with DB-level autocommit disabled and enabled. I'm using psycopg2 2.4.1 and PostgreSQL 9.0.4. I haven't tested other combinations of psycopg2, PostgreSQL, or other DBs.

Here are the two test settings files I used:

DATABASES = {
    'default': {
        'ENGINE': 'django.db.backends.postgresql_psycopg2',
        'NAME': 'djangotest',
        'USER': 'djangotest',
        'PASSWORD': 'djangotest',
    },
    'other': {
        'ENGINE': 'django.db.backends.postgresql_psycopg2',
        'NAME': 'djangotestother',
        'USER': 'djangotestother',
        'PASSWORD': 'djangotestother',
    }
}

DATABASES = {
    'default': {
        'ENGINE': 'django.db.backends.postgresql_psycopg2',
        'NAME': 'djangotest',
        'USER': 'djangotest',
        'PASSWORD': 'djangotest',
        'OPTIONS': {'autocommit': True},
    },
    'other': {
        'ENGINE': 'django.db.backends.postgresql_psycopg2',
        'NAME': 'djangotestother',
        'USER': 'djangotestother',
        'PASSWORD': 'djangotestother',
        'OPTIONS': {'autocommit': True},
    }
}

[kgibula]

Patch also applies to trunk and tests pass for PostgreSQL 8.4 and psycopg2 2.4.1. On other backends, however, 'DatabaseWrapper' object doesn't have an attribute 'isolation_level' so tests result in attribute errors (I tried MySQL and SQLite).

[Brodie Rao]

I'll attempt to revise the patch so the new tests don't blow up with other DBs.

[dikbrouwer]

Hi brodie, any updates on this patch? It seems to be critical to anybody who is using manual transaction management and PGBouncer.

[Anssi Kääriäinen]

I think I have nailed this one. The problem was that in _leave_transaction_management, the managed variable was telling if we were coming from a managed transaction. However, that is not interesting. The interesting thing is if we are going into still managed state or not.

I also rewrote the tests to fix enter/leave management stacking, and to work even if the settings do not contain the autocommit flag. This is a must in my opinion, as running the test suite with autcommit = True isn't something done regularly.

I left the supports_transactions fix out of this. However, why not just return True when using PostgreSQL, SQLite or Oracle?

I haven't ran the full test suite, which in this case seems necessary. I would like to mark this ready for committer (assuming tests pass), but I guess I am not allowed to do that... Would be nice to get this into 1.4.

[Brodie Rao]

It's been a while since I've looked at this issue, but let me see if I've got the whole picture right:

• Currently, autocommit isn't restored because the psycopg2 backend won't restore it unless it's already out of Django's "managed" mode. But managed mode is turned on when entering a transaction, so you get stuck at the higher isolation level.

• My patch removes the "not managed" check from _leave_transaction_management(), so it no longer cares whether you're in managed mode or not -- it'll go back to autocommit regardless.

• Your patch disables managed mode *before* calling _leave_transaction_management(), so managed will be False, and it will end up restoring autocommit.

I'm not entirely sure, but it seems like your patch makes more sense. That said, I don't think the try/except stuff you've added is necessary -- you should just move the "self._leave_transaction_management(self.is_managed())" call so it's after the existing self.transaction_state handling code.

Also, having looked at this code again, I don't think I fully understand the enter_transaction_mangement()'s managed parameter. It seems like if you're in autocommit mode and set it to False, you stay in autocommit mode. But after you've called that method, even if you set managed=False, self.is_managed() will still be True, and it's still going to try to restore autocommit mode in _leave_transaction_management() (which doesn't really matter, I guess).

I guess my real question is this: how does that managed parameter relate to is_managed()? They don't seem to be the same thing.

[Anssi Kääriäinen]

What happens is:

• when you enter transaction management "True" is appended to the transaction_state stack.
• when you leave transaction management, currently top of the stack (True) is given to _leave_transaction_management(). This tells "where we are coming from".
• my patch changes this in a way that the "True" is popped from the stack, and then what is left is given to _leave_transaction_management. Now, this is correct, because when setting the isolation level, we are not interested if we came from a managed block or not, we are interested if we are going into a managed state.
• ignoring totally the managed parameter is wrong, because you can stack enter/leave calls. This would lead into situation where the first leave always sets you into autocommit mode, no matter how many enter calls there have been before. This is tested in the patch.

About the managed parameter: don't ask. The transaction management code could be cleaner...

About the try-except: True, that change is not necessary, it just seemed better to me. "Pop the stack, it is an exception if it is empty".

I ran the full test suite and got some errors. However, I don't think they are related to this patch, but to some other recent commits. If you can review & mark the patch as ready for committer that would be great.

[Brodie Rao]

OK, I agree that your fix is the right approach.

I have a new patch series that incorporates your changes. The first patch fixes the supports_transactions DB feature being False when the test databases are using autocommit mode. The second patch is based on your patch, but I've moved database creation/deletion into setUp/tearDown methods and made more fine-grained tests in the test case. The entire test case is skipped if we aren't using psycopg2.

I also ran into an issue with your hardcoded isolation level constants. I think they might've changed across psycopg2 releases, so to be safe, I import them from psycopg2.

The patch series is based against trunk (at [17688]).

[Anssi Kääriäinen <akaariai@…>]

In [f572ee0c65ec5eac9edb0cb3e35c96ec86d89ffb]:

Fixed #16047 -- Restore autocommit state correctly on psycopg2

When the postgresql_psycopg2 backend was used with DB-level autocommit
mode enabled, after entering transaction management and then leaving
it, the isolation level was never set back to autocommit mode.

Thanks brodie for report and working on this issue.