[issue44958] [sqlite3] only reset statements when needed

[Erlend E. Aasland]

Change by Erlend E. Aasland :


--
pull_requests: +28589
pull_request: https://github.com/python/cpython/pull/30379

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue44958] [sqlite3] only reset statements when needed

[Pablo Galindo Salgado]

Pablo Galindo Salgado  added the comment:


New changeset 7b88f63e1dd4006b1a08b9c9f087dd13449ecc76 by Erlend Egeberg 
Aasland in branch 'main':
bpo-44958: Revert GH-27844 (GH-28574)
https://github.com/python/cpython/commit/7b88f63e1dd4006b1a08b9c9f087dd13449ecc76


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue44958] [sqlite3] only reset statements when needed

[Erlend E. Aasland]

Change by Erlend E. Aasland :


--
pull_requests: +26956
pull_request: https://github.com/python/cpython/pull/28574

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue44958] [sqlite3] only reset statements when needed

[Erlend E. Aasland]

Erlend E. Aasland  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue44958] [sqlite3] only reset statements when needed

[Erlend E. Aasland]

Erlend E. Aasland  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue44958] [sqlite3] only reset statements when needed

[Erlend E. Aasland]

Erlend E. Aasland  added the comment:

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

Correction: I _am_ able to reproduce this.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue44958] [sqlite3] only reset statements when needed

[Erlend E. Aasland]

Erlend E. Aasland  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue44958] [sqlite3] only reset statements when needed

[Erlend E. Aasland]

Erlend E. Aasland  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue44958] [sqlite3] only reset statements when needed

[Ken Jin]

Ken Jin  added the comment:

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

--
nosy: +kj

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue44958] [sqlite3] only reset statements when needed

[Erlend E. Aasland]

Erlend E. Aasland  added the comment:

> 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().

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue44958] [sqlite3] only reset statements when needed

[Erlend E. Aasland]

Erlend E. Aasland  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue44958] [sqlite3] only reset statements when needed

[miss-islington]

miss-islington  added the comment:


New changeset 3e3ff09058a71b92c32d1d7dc32470c0cf49300c by Erlend Egeberg 
Aasland in branch 'main':
bpo-44958: Fix ref. leak introduced in GH-27844 (GH-28490)
https://github.com/python/cpython/commit/3e3ff09058a71b92c32d1d7dc32470c0cf49300c


--
nosy: +miss-islington

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue44958] [sqlite3] only reset statements when needed

[Erlend E. Aasland]

Change by Erlend E. Aasland :


--
pull_requests: +26886
pull_request: https://github.com/python/cpython/pull/28490

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue44958] [sqlite3] only reset statements when needed

[Pablo Galindo Salgado]

Pablo Galindo Salgado  added the comment:


New changeset 050d1035957379d70e8601e6f5636637716a264b by Erlend Egeberg 
Aasland in branch 'main':
bpo-44958: Only reset `sqlite3` statements when needed (GH-27844)
https://github.com/python/cpython/commit/050d1035957379d70e8601e6f5636637716a264b


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue44958] [sqlite3] only reset statements when needed

[Erlend E. Aasland]

Erlend E. Aasland  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue44958] [sqlite3] only reset statements when needed

[Erlend E. Aasland]

Erlend E. Aasland  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue44958] [sqlite3] only reset statements when needed

[Erlend E. Aasland]

Change by Erlend E. Aasland :


--
Removed message: https://bugs.python.org/msg399931

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue44958] [sqlite3] only reset statements when needed

[Erlend E. Aasland]

Erlend E. Aasland  added the comment:

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 :)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue44958] [sqlite3] only reset statements when needed

[Erlend E. Aasland]

Change by Erlend E. Aasland :


--
keywords: +patch
pull_requests: +26307
stage:  -> patch review
pull_request: https://github.com/python/cpython/pull/27844

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue44958] [sqlite3] only reset statements when needed

[Erlend E. Aasland]

New submission from Erlend E. 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()
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 :)

--
components: Extension Modules
messages: 399931
nosy: berker.peksag, erlendaasland, pablogsal, serhiy.storchaka
priority: normal
severity: normal
status: open
title: [sqlite3] only reset statements when needed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com