[sqlite3] only reset statements when needed

[erlend-aasland]

BPO	44958
Nosy	@berkerpeksag, @serhiy-storchaka, @pablogsal, @miss-islington, @erlend-aasland, @Fidget-Spinner
PRs	bpo-44958: Only reset sqlite3 statements when needed #27844 bpo-44958: Fix ref. leak introduced in GH-27844 #28490 bpo-44958: Revert GH-27844 #28574 gh-89121: Keep the number of pending SQLite statements to a minimum #30379

^{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 = []

[erlend-aasland]

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:

1. before the first sqlite3_step() every time we (re)execute a query
2. 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]

I did a quick count of sqlite3_reset()s in the sqlite3 test suite:

• main: 2976 calls
• PR 27844: 1730 calls

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]

In msg399939, item 2 lacks one more "reset path":

2. at cursor exit, if there's an active statement

Rewording this to:

2. when a statement is removed from a cursor; that is either at cursor dealloc, or when the current statement is replaced.

[pablogsal]

New changeset 050d103 by Erlend Egeberg Aasland in branch 'main':
bpo-44958: Only reset sqlite3 statements when needed (GH-27844)
050d103

[miss-islington]

New changeset 3e3ff09 by Erlend Egeberg Aasland in branch 'main':
bpo-44958: Fix ref. leak introduced in #72031 (GH-28490)
3e3ff09

[erlend-aasland]

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]

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]

Erlend, I suspect that 050d103 may have introduced a perf regression in pyperformance's sqlite_synth benchmark:
https://speed.python.org/timeline/?exe=12&base=&ben=sqlite_synth&env=1&revs=50&equid=off&quarts=on&extr=on
The benchmark code is here https://github.com/python/pyperformance/blob/main/pyperformance/benchmarks/bm_sqlite_synth.py.

[erlend-aasland]

Ouch, that's quite a regression! Thanks for the heads up! I'll have a look at it right away.

[erlend-aasland]

I'm unable to reproduce this regression on my machine (macOS, debug build, no optimisations). Are you able to reproduce, Ken?

[erlend-aasland]

I'm unable to reproduce this regression on my machine (macOS, debug build, no optimisations) [...]

Correction: I _am_ able to reproduce this.

[erlend-aasland]

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]

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.