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.

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

bpo-45512: Simplify isolation_level handling in sqlite3 #29053

Merged
merged 13 commits into from
Nov 15, 2021

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Oct 19, 2021

Simplify reference handling and merge two connection object members:

  • Remove superfluous PyObject * member isolation_level from connection object
    (self->begin_statement carries the information we need)
  • Introduce get_begin_statement() helper for converting isolation level to begin statement
  • Simplify sqlite3.Connection.__init__ by using get_begin_statement
  • Use AC to remove reference handling for the isolation level parameter

https://bugs.python.org/issue45512

@erlend-aasland
Copy link
Contributor Author

FYI: sqlite3_strnicmp is documented here: https://sqlite.org/c3ref/stricmp.html

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Oct 25, 2021

Some more information about these changes, for the reviewers convenience. In addition to the refactoring needed to merge pysqlite_Connection members isolation_level and begin_statement, this PR has the following side effect:

During sqlite3.connection.__init__, an implicit COMMIT is no longer issued for connections with isolation_level=None. This is ok, since a database connection starts with autocommit mode on by default. Quoting from the SQLite docs: "Autocommit mode is on by default. Autocommit mode is disabled by a BEGIN statement. Autocommit mode is re-enabled by a COMMIT or ROLLBACK."

@erlend-aasland
Copy link
Contributor Author

Dang, there's a ref. leak. I'll ping you when I've found it, @corona10.

@erlend-aasland
Copy link
Contributor Author

Finally got around to revisit this; fixed the ref. leak, @corona10. Does this look ok to you?

@corona10
Copy link
Member

corona10 commented Nov 5, 2021

Finally got around to revisit this; fixed the ref. leak, @corona10. Does this look ok to you?

I will take a look by next week

@erlend-aasland
Copy link
Contributor Author

@corona10 I added 4b4ba4f. Does that help to explain the connection between begin statements and isolation level?

@corona10
Copy link
Member

corona10 commented Nov 12, 2021

Does that help to explain the connection between begin statements and isolation level?

Thanks, I will take a look one more time :)

@corona10 corona10 merged commit b567b9d into python:main Nov 15, 2021
@erlend-aasland erlend-aasland deleted the sqlite-isolation-level branch November 15, 2021 08:42
@erlend-aasland
Copy link
Contributor Author

Thank you for reviewing, Dong-hee!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants