Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix possible bugs when setting sqlite3.Connection.isolation_level #72068

Closed
zhangyangyu opened this issue Aug 27, 2016 · 14 comments
Closed

Fix possible bugs when setting sqlite3.Connection.isolation_level #72068

zhangyangyu opened this issue Aug 27, 2016 · 14 comments
Assignees
@serhiy-storchaka
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@zhangyangyu
Copy link
Member

BPO 27881
Nosy @berkerpeksag, @serhiy-storchaka, @zhangyangyu, @palaviv
Files
  • issue27881.patch
  • issue27881_v2.patch
  • issue27881_v3.patch
  • issue27881_v4.patch
  • issue27881_v5.patch
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2016-09-08.13:24:25.345>
created_at = <Date 2016-08-27.19:21:29.390>
labels = ['type-bug', 'library']
title = 'Fix possible bugs when setting sqlite3.Connection.isolation_level'
updated_at = <Date 2016-09-08.13:24:25.344>
user = 'https://github.com/zhangyangyu'

    bugs.python.org fields:

    activity = <Date 2016-09-08.13:24:25.344>
actor = 'berker.peksag'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2016-09-08.13:24:25.345>
closer = 'berker.peksag'
components = ['Library (Lib)']
creation = <Date 2016-08-27.19:21:29.390>
creator = 'xiang.zhang'
dependencies = []
files = ['44247', '44265', '44266', '44318', '44328']
hgrepos = []
issue_num = 27881
keywords = ['patch']
message_count = 14.0
messages = ['273796', '273820', '273909', '273910', '273914', '274068', '274088', '274100', '274101', '274158', '274163', '274168', '274169', '275013']
nosy_count = 6.0
nosy_names = ['python-dev', 'berker.peksag', 'serhiy.storchaka', 'xiang.zhang', 'palaviv', 'tehybel']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue27881'
versions = ['Python 3.5', 'Python 3.6']
    @zhangyangyu
    Copy link
    Member Author

    This is a follow up of bpo-27861 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.

    @zhangyangyu zhangyangyu added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Aug 27, 2016
    @zhangyangyu
    Copy link
    Member Author

    bpo-27881.patch tires to solve this. Hope to get feedback.

    @serhiy-storchaka serhiy-storchaka self-assigned this Aug 28, 2016
    @zhangyangyu
    Copy link
    Member Author

    Serhiy, I've updated the patch. Looking forward to your feedback.

    @serhiy-storchaka
    Copy link
    Member

    In general LGTM. But added several style comments.

    @zhangyangyu
    Copy link
    Member Author

    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.

    @palaviv
    Copy link
    Mannequin

    palaviv mannequin commented Aug 31, 2016 

    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.

    @zhangyangyu
    Copy link
    Member Author

    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.

    @serhiy-storchaka
    Copy link
    Member

    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.

    @zhangyangyu
    Copy link
    Member Author

    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.

    @palaviv
    Copy link
    Mannequin

    palaviv mannequin commented Sep 1, 2016

    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.

    @serhiy-storchaka
    Copy link
    Member

    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.

    @palaviv
    Copy link
    Mannequin

    palaviv mannequin commented Sep 1, 2016

    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.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 1, 2016

    New changeset 546b1f70cbed by Serhiy Storchaka in branch '3.5':
    Issue bpo-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 bpo-27881: Fixed possible bugs when setting sqlite3.Connection.isolation_level.
    https://hg.python.org/cpython/rev/96e05f1af2c8

    @berkerpeksag
    Copy link
    Member

    Closing this as 'fixed'. Any further improvement can be discussed in separate issues. Thanks!

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees

    @serhiy-storchaka serhiy-storchaka

    Labels
    stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    3 participants
    @berkerpeksag @serhiy-storchaka @zhangyangyu