| msg273796 - (view) |
Author: Xiang Zhang (xiang.zhang) *  |
Date: 2016-08-27 19:21 |
| This is a follow up of 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) * |
Date: 2016-08-28 10:29 |
| .patch tires to solve this. Hope to get feedback. |
|
|
| msg273909 - (view) |
Author: Xiang Zhang (xiang.zhang) * |
Date: 2016-08-30 11:02 |
| Serhiy, I've updated the patch. Looking forward to your feedback. |
|
|
| msg273910 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2016-08-30 11:20 |
| In general LGTM. But added several style comments. |
|
|
| msg273914 - (view) |
Author: Xiang Zhang (xiang.zhang) * |
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) * |
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) * |
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) * |
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) * |
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)  |
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) * |
Date: 2016-09-08 13:24 |
| Closing this as 'fixed'. Any further improvement can be discussed in separate issues. Thanks! |
|
|