classification
Title: Fix possible bugs when setting sqlite3.Connection.isolation_level
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.6, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: berker.peksag, palaviv, python-dev, serhiy.storchaka, tehybel, xiang.zhang
Priority: normal Keywords: patch

Created on 2016-08-27 19:21 by xiang.zhang, last changed 2016-09-08 13:24 by berker.peksag. This issue is now closed.

Files
File name Uploaded Description Edit
issue27881.patch xiang.zhang, 2016-08-28 10:29 review
issue27881_v2.patch xiang.zhang, 2016-08-30 11:02 review
issue27881_v3.patch xiang.zhang, 2016-08-30 12:47 review
issue27881_v4.patch serhiy.storchaka, 2016-09-01 08:02 review
issue27881_v5.patch palaviv, 2016-09-01 17:56 review
Messages (14)
msg273796 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-08-27 19:21
This is a follow up of issue27861 and means to fix the second issue mentioned in it.

pysqlite_connection_set_isolation_level now does not check allowed values and when any part fails, it leaves the Connection object in an inconsistent state.

I'll write a patch trying to fix this tomorrow.
msg273820 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-08-28 10:29
issue27881.patch tires to solve this. Hope to get feedback.
msg273909 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-08-30 11:02
Serhiy, I've updated the patch. Looking forward to your feedback.
msg273910 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-08-30 11:20
In general LGTM. But added several style comments.
msg273914 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-08-30 12:47
Leave replies and make a trival change in v3. I don't change the other two just as I state in the replies. But please do what you like.
msg274068 - (view) Author: Aviv Palivoda (palaviv) * Date: 2016-08-31 20:38
Xiang what do you think about changing the isolation_levels list to begin_statements list:
static const char * const begin_statements[] = {"BEGIN", "BEGIN DEFERRED", "BEGIN IMMEDIATE", "BEGIN EXCLUSIVE", NULL};

This change will allow you to remove all the strings concatenations and the malloc/free.
msg274088 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-09-01 02:47
I thought about this method but didn't find it's simpler.

You cannot avoid malloc/free since the isolation level has to be on heap. It's going to be freed in the dealloc method unless your alter that part too. Then it's about one memcpy or two, which doesn't make much difference. And in this method you have to do more in argument checking. A simple strstr doesn't solve this problem.
msg274100 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-09-01 08:02
> Xiang what do you think about changing the isolation_levels list to begin_statements list

This is interesting suggestion. Indeed, this simplifies the code. Thanks Aviv.
msg274101 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-09-01 08:13
Hmm, you do this "It's going to be freed in the dealloc method unless your alter that part too". If this is done I admit this is more clean. Patch LGTM.
msg274158 - (view) Author: Aviv Palivoda (palaviv) * Date: 2016-09-01 17:56
What do you think about removing the isolation_level member from the pysqlite_Connection. It is only there to be for the pysqlite_connection_get_isolation_level and that could be easily calculated from the begin_statement.
msg274163 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-09-01 18:25
Yes, I thought about this. This changes the behavior (for now isolation_level returns exactly the same object that was assigned to it) and slows down the getter. This can be added only on the default branch.
msg274168 - (view) Author: Aviv Palivoda (palaviv) * Date: 2016-09-01 18:49
The only change I see that can happen is that we return upper case isolation level when the user used a lower case. I think that it is worth to slow down the getter for the code simplification.
msg274169 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-09-01 19:22
New changeset 546b1f70cbed by Serhiy Storchaka in branch '3.5':
Issue #27881: Fixed possible bugs when setting sqlite3.Connection.isolation_level.
https://hg.python.org/cpython/rev/546b1f70cbed

New changeset 96e05f1af2c8 by Serhiy Storchaka in branch 'default':
Issue #27881: Fixed possible bugs when setting sqlite3.Connection.isolation_level.
https://hg.python.org/cpython/rev/96e05f1af2c8
msg275013 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2016-09-08 13:24
Closing this as 'fixed'. Any further improvement can be discussed in separate issues. Thanks!
History
Date User Action Args
2016-09-08 13:24:25berker.peksagsetstatus: open -> closed
resolution: fixed
messages: + msg275013

stage: patch review -> resolved
2016-09-01 19:22:06python-devsetnosy: + python-dev
messages: + msg274169
2016-09-01 18:49:41palavivsetmessages: + msg274168
2016-09-01 18:25:39serhiy.storchakasetmessages: + msg274163
2016-09-01 17:56:37palavivsetfiles: + issue27881_v5.patch

messages: + msg274158
2016-09-01 08:13:32xiang.zhangsetmessages: + msg274101
2016-09-01 08:02:29serhiy.storchakasetfiles: + issue27881_v4.patch

messages: + msg274100
2016-09-01 02:47:09xiang.zhangsetmessages: + msg274088
2016-08-31 20:38:14palavivsetnosy: + palaviv
messages: + msg274068
2016-08-30 12:47:29xiang.zhangsetfiles: + issue27881_v3.patch

messages: + msg273914
2016-08-30 11:20:53serhiy.storchakasetmessages: + msg273910
2016-08-30 11:02:07xiang.zhangsetfiles: + issue27881_v2.patch

messages: + msg273909
2016-08-28 10:38:18serhiy.storchakasetassignee: serhiy.storchaka
stage: patch review
2016-08-28 10:29:14xiang.zhangsetfiles: + issue27881.patch
keywords: + patch
messages: + msg273820
2016-08-27 19:21:29xiang.zhangcreate