chornbeak / pyodbc

Automatically exported from code.google.com/p/pyodbc
MIT No Attribution
0 stars 0 forks source link

Connection/cursor context managers force commits on exit #397

Open GoogleCodeExporter opened 8 years ago

GoogleCodeExporter commented 8 years ago
What steps will reproduce the problem?
1. Create a connection to a database with autocommit off using the context 
manager, and then leave an SQL statement uncommitted.  For example as follows:

import pyodbc

# Note, auto-commit is set False on the connection
with pyodbc.connect('<...>', autocommit=False) as conn, \
     conn.cursor() as cursor:

    cursor.execute(r"CREATE TABLE ContextManagerCommitTest " \
                   r"(FirstName  varchar(100) NULL, " \
                    r"MiddleName varchar(100) NULL, " \
                    r"LastName   varchar(100) NULL)")
    conn.commit()

    cursor.execute(r"INSERT INTO BatchUpdaterTest " \
                     r"(FirstName, MiddleName, LastName) " \
                   r"VALUES " \
                      r"('FN1', 'MN1', 'LN1'), " \
                      r"('FN2', 'MN2', 'LN2'), " \
                      r"('FN3', 'MN3', 'LN3')")
    # Note, this insert transaction is NOT committed

2. After this code has run, examine the table.  It should be empty but isn't.

What is the expected output? What do you see instead?
The table is populated with the three records.  This is absolutely NOT what I 
would expect when auto-commit is off.  For example, suppose I am running 
multiple SQL updates against the database, and I want to commit all of them in 
one go or none of them.  If an exception occurs half way through (for whatever 
reason), pyodbc will issue a commit regardless when the context manager exits.  
This is really bad because pyodbc is committing only half my updates and 
potentially leaving the database in an odd state.  If autocommit is off for the 
connection, it is entirely the responsibility of the programmer to commit 
transactions.  Pyodbc should never do a commit without being told to do so.
Right now, if I want to use a connection with autocommit off, I must remember 
not to use the context manager because otherwise there is a significant risk of 
screwing up my data.

What version of the product are you using? On what operating system?
Version 3.0.7, Python 3.4, on Windows 7 against a SQL Server 2008 R2 database.

Please provide any additional information below.

It appears that both connection.cpp and cursor.cpp do a commit in their 
"__exit__" functions.  A quick fix may be to change the "cnxn->nAutoCommit" 
check from SQL_AUTOCOMMIT_OFF to SQL_AUTOCOMMIT_ON, which seems more logical 
regardless.

Original issue reported on code.google.com by toastie...@gmail.com on 12 Apr 2015 at 5:34

GoogleCodeExporter commented 8 years ago
On further investigation, it appears that uncommitted transactions ARE rolled 
back if an exception occurs, but if no exception occurs and the context is 
exited normally then the transaction IS committed.  So what I was saying in my 
previous message about the exception scenario wasn't correct.

Nevertheless, my original test is still relevant and this is still not good 
behavior.  There are times when I want to hand a connection object to another 
module to do some updates, but maintain control over commits.  If the other 
module is unknowingly committing transactions (by using a context manager), 
this is not good.

Original comment by toastie...@gmail.com on 12 Apr 2015 at 7:31

GoogleCodeExporter commented 8 years ago
I have created a git branch to give a suggestion for how this could be fixed:
https://github.com/keitherskine/pyodbc/commit/6c0a9d18e496cee00ba8d412db3a1b72e8
ee78de

The changes are as follows:

1) In connection.cpp, Connection_exit() commits only when auto-commit is on and 
it's a normal (non-error) exit, otherwise it rolls back the transaction.  I 
believe this is the correct behavior.
I also added a call to Connection_close() at the end.  Without it, I'm not sure 
how the connection is being properly closed.

In cursor.cpp, I have removed all attempts to commit/rollback.  When a cursor 
is closed, my understanding is that there is no requirement or implication that 
transactions will be committed or rolled back at the same time.  I'm puzzled 
why this is happening in this function.  Nevertheless, the cursor should still 
be closed, hence I've added a call to Cursor_close().

I've also added some tests to cover these changes.

If it helps to turn this into a pull request, let me know.

Original comment by toastie...@gmail.com on 12 Apr 2015 at 11:06