bpo-32215: Fix performance regression in sqlite3 #8511

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

Merged

berkerpeksag merged 1 commit into python:master from berkerpeksag:32215-sqlite-performance

Sep 20, 2018

Member

berkerpeksag commented Jul 28, 2018 •

edited by bedevere-bot

Loading

https://bugs.python.org/issue32215


          bpo-32215: Fix performance regression in sqlite3

9945e20

berkerpeksag added needs backport to 3.6 labels

the-knights-who-say-ni added the CLA signed label

bedevere-bot added the awaiting merge label

serhiy-storchaka reviewed

View reviewed changes

Member

serhiy-storchaka left a comment

Would be nice to add tests.

Is the tab character recognized as a whitespace by Sqlite?

Modules/_sqlite/statement.c

    
                                  || (PyOS_strnicmp(p, "update ", 7) == 0)

                                  || (PyOS_strnicmp(p, "delete ", 7) == 0)

                                  || (PyOS_strnicmp(p, "replace ", 8) == 0);

                      self->is_dml = (PyOS_strnicmp(p, "insert", 6) == 0)

Member

serhiy-storchaka Jul 28, 2018

Are there commands that start with "update", "delete", etc? Can a user function that starts with such prefix (for example "replacewhitespaces()") be used in this context?

Member Author

berkerpeksag Jul 28, 2018

Can a user function that starts with such prefix (for example "replacewhitespaces()") be used in this context?

Yes, they can. The original intent of the old version was to make that check more robust and avoid cases like you've mentioned. I don't think there is a way to achieve this without relying on SQLite APIs, but then relying on newer APIs usually results seeing "I can't compile the sqlite3 module on my system" issues on the tracker :)

Member Author

berkerpeksag commented Jul 28, 2018

Would be nice to add tests.

To test that the performance regression is fixed or to test that something like a user defined function (e.g. replacewhitespaces()) doesn't cause any problems?

Is the tab character recognized as a whitespace by Sqlite?

Looking at https://github.com/mackyle/sqlite/blob/master/src/tokenize.c it does.

Member

serhiy-storchaka commented Jul 28, 2018

First, test that \t and \n instead of space are accepted. Is there any effect besides the performance regression?

Testing a user function starting with one of these prefixes would be nice too.

Member Author

berkerpeksag commented Sep 20, 2018

I'm going to merge this without tests to get it in for 3.7.1 and open a separate pull request for tests.

berkerpeksag merged commit 8d1e190 into python:master

Contributor

miss-islington commented Sep 20, 2018

Thanks @berkerpeksag for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7.
🐍🍒⛏🤖

bedevere-bot removed the awaiting merge label

berkerpeksag deleted the 32215-sqlite-performance branch

September 20, 2018 11:10

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request


          bpo-32215: Fix performance regression in sqlite3 (pythonGH-8511)

4bc8337

(cherry picked from commit 8d1e190)

Co-authored-by: Berker Peksag <[email protected]>

bedevere-bot commented Sep 20, 2018

GH-9441 is a backport of this pull request to the 3.7 branch.

bedevere-bot removed the needs backport to 3.7 label

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request


          bpo-32215: Fix performance regression in sqlite3 (pythonGH-8511)

d8599fa

(cherry picked from commit 8d1e190)

Co-authored-by: Berker Peksag <[email protected]>

bedevere-bot commented Sep 20, 2018

GH-9442 is a backport of this pull request to the 3.6 branch.

bedevere-bot removed the needs backport to 3.6 label

berkerpeksag added the needs backport to 3.7 label

Contributor

miss-islington commented Sep 20, 2018

Thanks @berkerpeksag for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request


          bpo-32215: Fix performance regression in sqlite3 (pythonGH-8511)

012f27f

(cherry picked from commit 8d1e190)

Co-authored-by: Berker Peksag <[email protected]>

bedevere-bot commented Sep 20, 2018

GH-9449 is a backport of this pull request to the 3.7 branch.

bedevere-bot removed the needs backport to 3.7 label

berkerpeksag added a commit that referenced this pull request


          bpo-32215: Fix performance regression in sqlite3 (GH-8511)

015cd0f

(cherry picked from commit 8d1e190)

Co-authored-by: Berker Peksag <[email protected]>

berkerpeksag added the needs backport to 3.6 label

Contributor

miss-islington commented Sep 20, 2018

Thanks @berkerpeksag for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request


          bpo-32215: Fix performance regression in sqlite3 (pythonGH-8511)

14a1a10

(cherry picked from commit 8d1e190)

Co-authored-by: Berker Peksag <[email protected]>

bedevere-bot commented Sep 20, 2018

GH-9452 is a backport of this pull request to the 3.6 branch.

bedevere-bot removed the needs backport to 3.6 label

berkerpeksag added a commit that referenced this pull request


          bpo-32215: Fix performance regression in sqlite3 (GH-8511)

4fb672f

(cherry picked from commit 8d1e190)

Co-authored-by: Berker Peksag <[email protected]>

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

Labels

None yet