GeezerVison is showing github.com — [sqlite3] only reset statements when needed · Issue #89121 · python/cpython
[sqlite3] only reset statements when needed #89121Description
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 = None
closed_at = None
created_at = <Date 2021-08-19.20:05:09.432>
labels = ['extension-modules']
title = '[sqlite3] only reset statements when needed'
updated_at = <Date 2022-01-03.23:42:52.964>
user = 'https://github.com/erlend-aasland' bugs.python.org fields:
activity = <Date 2022-01-03.23:42:52.964>
actor = 'erlendaasland'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Extension Modules']
creation = <Date 2021-08-19.20:05:09.432>
creator = 'erlendaasland'
dependencies = []
files = []
hgrepos = []
issue_num = 44958
keywords = ['patch']
message_count = 14.0
messages = ['399939', '399984', '401186', '402312', '402317', '402319', '402402', '402423', '402424', '402425', '402427', '402490', '402677', '402683']
nosy_count = 6.0
nosy_names = ['berker.peksag', 'serhiy.storchaka', 'pablogsal', 'miss-islington', 'erlendaasland', 'kj']
pr_nums = ['27844', '28490', '28574', '30379']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = None
url = 'https://bugs.python.org/issue44958'
versions = [] Activityerlend-aasland commented on Aug 20, 2021 ContributorAuthor More actions Ref. Serhiy's msg387858 in bpo-43350:
"Maybe the code could be rewritten in more explicit way and call pysqlite_statement_reset() only when it is necessary [...]"
Currently, we try to reset statements in all "statement exit" routes. IMO, it would be cleaner to just reset statements when we really need to:
- before the first sqlite3_step() every time we (re)execute a query
- at cursor exit, if there's an active statement
(3. in pysqlite_do_all_statements() ... see bpo-44092)
This will make the code easier to follow, and it will minimise the number of resets. The current patch is pretty small: 7 insertions(+), 33 deletions(-)
Pro:
- less lines of code, less maintenance
- cleaner exit paths
- optimise SQLite API usage
Con:
- code churn
If this is accepted, PR 25984 of bpo-44073 will be easier to land and review :) erlend-aasland commented on Aug 20, 2021 ContributorAuthor More actions I did a quick count of sqlite3_reset()s in the sqlite3 test suite:
Since we never call sqlite3_reset() with a NULL pointer, all sqlite3_reset() calls we execute hold the SQLite db mutex; reducing the number of sqlite3_reset() calls reduces the number of times we need to hold the mutex. erlend-aasland commented on Sep 6, 2021 ContributorAuthor More actions In msg399939, item 2 lacks one more "reset path":
- at cursor exit, if there's an active statement
Rewording this to:
- when a statement is removed from a cursor; that is either at cursor dealloc, or when the current statement is replaced.
pablogsal commented on Sep 21, 2021 miss-islington commented on Sep 21, 2021 erlend-aasland commented on Sep 21, 2021 ContributorAuthor More actions Now that pysqlite_statement_reset() is only used in cursor.c, I suggest to move it to cursor.c and make it a static function. erlend-aasland commented on Sep 21, 2021 ContributorAuthor More actions
Now that pysqlite_statement_reset() is only used in cursor.c, I suggest to move it to cursor.c and make it a static function.
I'll wait until PR 25984 is merged before opening a PR for relocating pysqlite_statement_reset(). Fidget-Spinner commented on Sep 22, 2021 erlend-aasland commented on Sep 22, 2021 ContributorAuthor More actions Ouch, that's quite a regression! Thanks for the heads up! I'll have a look at it right away. erlend-aasland commented on Sep 22, 2021 ContributorAuthor More actions I'm unable to reproduce this regression on my machine (macOS, debug build, no optimisations). Are you able to reproduce, Ken? erlend-aasland commented on Sep 22, 2021 ContributorAuthor More actions
I'm unable to reproduce this regression on my machine (macOS, debug build, no optimisations) [...]
Correction: I _am_ able to reproduce this. erlend-aasland commented on Sep 23, 2021 ContributorAuthor More actions Explicitly resetting statements when we're done with them removes the performance regression; SQLite works more efficient when we keep the number of non-reset statements low. erlend-aasland commented on Sep 26, 2021 ContributorAuthor More actions I'll revert PR 27844 for now (except the tests).
Since SQLite works better when we keep the number of non-reset statements to a minimum, we need to ensure that we reset statements when we're done with them (sqlite3_step() returns SQLITE_DONE or an error). Before doing such a change, we should clean up _pysqlite_query_execute() so we don't need to sprinkle that function with pysqlite_statement_reset's. I plan to do this before attempting to clean up reset usage again. pablogsal commented on Sep 26, 2021 added a commit that references this issue on Jun 23, 2022gh-89121: Keep the number of pending SQLite statements to a minimum (#…
MetadataMetadataStatusShow more project fields
DevelopmentNo branches or pull requestsIssue actions
|