This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: [sqlite3] simplify "isolation level"
Type: Stage: resolved
Components: Extension Modules Versions: Python 3.11
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: corona10, erlendaasland
Priority: low Keywords: patch

Created on 2021-10-18 12:59 by erlendaasland, last changed 2022-04-11 14:59 by admin. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 29053 merged erlendaasland, 2021-10-19 09:00
PR 29561 merged erlendaasland, 2021-11-15 09:53
PR 29562 merged corona10, 2021-11-15 13:11
PR 29576 merged erlendaasland, 2021-11-16 12:03
PR 29593 merged erlendaasland, 2021-11-17 13:04
Messages (12)
msg404182 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-10-18 12:59
While working on bpo-45126 / GH-28227 (and while working on the AC conversion, bpo-40956), I've been slightly frustrated on the implementation of sqlite3 "isolation level". The code is messy, and we've got two connection members that carry pretty much the same type of information (self->isolation_level, and self->begin_statement).

I would like to make the following enhancements:
  - merge 'isolation_level' and 'begin_statement' members in some kind of way
  - split pysqlite_connection_set_isolation_level():
    + one method for parsing and storing the 'isolation_level' member
    + one method for carrying out any needed SQLite API operation

This should result in a cleaner connection __init__() method.


Another slightly related performance enhancement could be to cache the "begin" (and "commit") statements as sqlite3_stmt pointers on the connection object, but that is a digression.
msg405816 - (view) Author: Dong-hee Na (corona10) * (Python committer) Date: 2021-11-05 18:33
Hmm, now I understood what you intended,
The code was hard to read without knowing the condition of the begin_statement.
Would this be a trade-off in terms of code readability?
msg405936 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-11-08 08:51
IMO, GH-29053 improves readability, mainly because of these factors:

- Argument Clinic now takes care of the ref count dance, and as a bonus, it is more self-documenting (stating clearly that it accepts str and None)
- Begin statement validation has been refactored out, which IMO helps understanding how isolation level and begin statement is related
- The isolation level setter is now greatly simplified; it fits well in a screenful, there are fewer ref count dances
- Using get_begin_statement() is much less noisier than the previous string compare code in pysqlite_connection_set_isolation_level()


The only thing that still bugs me is the "magical" 6 byte offset needed to convert between isolation level and begin statement. Perhaps a set of conversion functions would make it more readable.
msg406342 - (view) Author: Dong-hee Na (corona10) * (Python committer) Date: 2021-11-15 07:51
New changeset b567b9d74bd9e476a3027335873bb0508d6e450f by Erlend Egeberg Aasland in branch 'main':
bpo-45512: Simplify isolation_level handling in `sqlite3` (GH-29053)
https://github.com/python/cpython/commit/b567b9d74bd9e476a3027335873bb0508d6e450f
msg406347 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-11-15 10:09
Need to amend GH-29053, so I'm opening until the fix is merged.
msg406350 - (view) Author: Dong-hee Na (corona10) * (Python committer) Date: 2021-11-15 12:55
New changeset 822c3dcce3996e411c1ff5c432c6ac7d2845cfd6 by Erlend Egeberg Aasland in branch 'main':
bpo-45512: Raise exception if sqlite3.Connection.__init__ is called with bad isolation level (#29561)
https://github.com/python/cpython/commit/822c3dcce3996e411c1ff5c432c6ac7d2845cfd6
msg406463 - (view) Author: Dong-hee Na (corona10) * (Python committer) Date: 2021-11-17 10:01
New changeset 5f9247e36a0213b0dcfd43533db5cf6570895cfd by Erlend Egeberg Aasland in branch 'main':
bpo-45512: Extend `sqlite3` test suite regarding isolation levels (GH-29576)
https://github.com/python/cpython/commit/5f9247e36a0213b0dcfd43533db5cf6570895cfd
msg406467 - (view) Author: Dong-hee Na (corona10) * (Python committer) Date: 2021-11-17 12:47
@erlendaasland

Now we close this issue right?
msg406468 - (view) Author: Dong-hee Na (corona10) * (Python committer) Date: 2021-11-17 12:47
New changeset e002bbc6cce637171fb2b1391ffeca8643a13843 by Dong-hee Na in branch 'main':
bpo-45512: Simplify manage isolation level (GH-29562)
https://github.com/python/cpython/commit/e002bbc6cce637171fb2b1391ffeca8643a13843
msg406469 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-11-17 12:57
> Now we close this issue right?

Not yet ;) I have an AC tweak I'd like to add. Wait for a PR anytime soon.
msg406522 - (view) Author: Dong-hee Na (corona10) * (Python committer) Date: 2021-11-18 09:18
New changeset 0920b61a0cb30128287ebafab1df8cad3a3dffdb by Erlend Egeberg Aasland in branch 'main':
bpo-45512: Use Argument Clinic to set sqlite3 isolation level (GH-29593)
https://github.com/python/cpython/commit/0920b61a0cb30128287ebafab1df8cad3a3dffdb
msg406529 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-11-18 10:28
Thanks for reviews and improvements, Dong-hee :)
History
Date User Action Args
2022-04-11 14:59:51adminsetgithub: 89675
2021-11-18 10:28:27erlendaaslandsetstatus: open -> closed
resolution: fixed
messages: + msg406529

stage: patch review -> resolved
2021-11-18 09:18:16corona10setmessages: + msg406522
2021-11-17 13:04:20erlendaaslandsetpull_requests: + pull_request27836
2021-11-17 12:57:40erlendaaslandsetmessages: + msg406469
2021-11-17 12:47:39corona10setmessages: + msg406468
2021-11-17 12:47:34corona10setmessages: + msg406467
2021-11-17 10:01:59corona10setmessages: + msg406463
2021-11-16 12:03:19erlendaaslandsetpull_requests: + pull_request27820
2021-11-15 13:11:00corona10setstage: resolved -> patch review
pull_requests: + pull_request27810
2021-11-15 12:55:42corona10setmessages: + msg406350
2021-11-15 10:09:37erlendaaslandsetstatus: closed -> open
resolution: fixed -> (no value)
messages: + msg406347
2021-11-15 09:53:53erlendaaslandsetpull_requests: + pull_request27809
2021-11-15 08:44:42erlendaaslandsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2021-11-15 07:51:17corona10setmessages: + msg406342
2021-11-08 08:51:09erlendaaslandsetmessages: + msg405936
2021-11-05 18:33:16corona10setnosy: + corona10
messages: + msg405816
2021-10-19 09:00:36erlendaaslandsetkeywords: + patch
stage: patch review
pull_requests: + pull_request27325
2021-10-18 12:59:11erlendaaslandcreate