[issue43350] [sqlite3] Active statements are reset twice in _pysqlite_query_execute()

[Erlend E. Aasland]

Erlend E. Aasland  added the comment:

msg387858 (Serhiy):
> Maybe the code could be rewritten in more explicit way and call
> pysqlite_statement_reset() only when it is necessary [...]

I've opened bpo-44958 for such an enhancement. Closing this issue.

--
resolution:  -> duplicate
stage: patch review -> resolved
status: open -> closed
superseder:  -> [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

[issue43350] [sqlite3] Active statements are reset twice in _pysqlite_query_execute()

[Erlend E. Aasland]

Erlend E. Aasland  added the comment:

Grep for SECONDRESET in log.txt to get the complete "API context".

As far as I can see, there is no harm in removing the redundant reset statement.

--

___
Python tracker 

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

[issue43350] [sqlite3] Active statements are reset twice in _pysqlite_query_execute()

[Erlend E. Aasland]

Erlend E. Aasland  added the comment:

Complete fprintf log added, for reference.

--
Added file: https://bugs.python.org/file50033/log.txt

___
Python tracker 

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

[issue43350] [sqlite3] Active statements are reset twice in _pysqlite_query_execute()

[Erlend E. Aasland]

Erlend E. Aasland  added the comment:

fprintf debugging patch added, for reference

--
Added file: https://bugs.python.org/file50032/fprintf.diff

___
Python tracker 

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

[issue43350] [sqlite3] Active statements are reset twice in _pysqlite_query_execute()

[Erlend E. Aasland]

Erlend E. Aasland  added the comment:

Adding fprintf's in pysqlite_statement_reset:
diff --git a/Modules/_sqlite/statement.c b/Modules/_sqlite/statement.c
--- a/Modules/_sqlite/statement.c
+++ b/Modules/_sqlite/statement.c
@@ -347,19 +363,23 @@
 int pysqlite_statement_reset(pysqlite_Statement* self)
 {
 int rc;
 
 rc = SQLITE_OK;
 
 if (self->in_use && self->st) {
 Py_BEGIN_ALLOW_THREADS
 rc = sqlite3_reset(self->st);
+fprintf(stderr, "sqlite3_reset(stmt=%p) => %d: %s\n",
+self->st, rc, sqlite3_errstr(rc));
 Py_END_ALLOW_THREADS
 
 if (rc == SQLITE_OK) {
 self->in_use = 0;
 }
+} else {
+fprintf(stderr, "sqlite3_reset => noop\n");
 }
 
 return rc;
 }
 

In Modules/_sqlite/cursor.c, I've also added a fprintf(stderr, "SECONDRESET: 
"); before the code in question.


$ ./python.exe -m test test_sqlite  2> log.txt
$ cat log.txt | grep -A1 -B1 SECONDRESET
sqlite3_reset(stmt=0x7fc360177e98) => 0: not an error
SECONDRESET: sqlite3_reset => noop
sqlite3_reset => noop
--
--
sqlite3_reset(stmt=0x7fc3a005f9e8) => 0: not an error
SECONDRESET: sqlite3_reset => noop

--
--
sqlite3_reset(stmt=0x7fc3a005ebd8) => 0: not an error
SECONDRESET: sqlite3_reset => noop

--
--
sqlite3_reset(stmt=0x7fc3a005f538) => 0: not an error
SECONDRESET: sqlite3_reset => noop

--
--
sqlite3_reset(stmt=0x7fc3a005f9e8) => 0: not an error
SECONDRESET: sqlite3_reset => noop

--
--
sqlite3_reset(stmt=0x7fc3a005f538) => 0: not an error
SECONDRESET: sqlite3_reset => noop

--
--
sqlite3_reset(stmt=0x7fc3a005f9e8) => 0: not an error
SECONDRESET: sqlite3_reset => noop

--
--
sqlite3_reset(stmt=0x7fc35000fe98) => 0: not an error
SECONDRESET: sqlite3_reset => noop

--
--
sqlite3_reset(stmt=0x7fc3500107f8) => 0: not an error
SECONDRESET: sqlite3_reset => noop

--
--
sqlite3_reset(stmt=0x7fc350010348) => 0: not an error
SECONDRESET: sqlite3_reset => noop

--
--
sqlite3_reset(stmt=0x7fc3500107f8) => 0: not an error
SECONDRESET: sqlite3_reset => noop
sqlite3_reset => noop
--
--
sqlite3_reset(stmt=0x7fc350010348) => 0: not an error
SECONDRESET: sqlite3_reset => noop
sqlite3_reset => noop
--
--
sqlite3_reset => noop
SECONDRESET: sqlite3_reset => noop

--
--
sqlite3_reset(stmt=0x7fc3700287f8) => 0: not an error
SECONDRESET: sqlite3_reset => noop

--
--
sqlite3_reset => noop
SECONDRESET: sqlite3_reset => noop

--
--
sqlite3_reset => noop
SECONDRESET: sqlite3_reset => noop
sqlite3_reset => noop
--
--
sqlite3_reset => noop
SECONDRESET: sqlite3_reset => noop
sqlite3_reset => noop
--
--
sqlite3_reset => noop
SECONDRESET: sqlite3_reset => noop
sqlite3_reset => noop
--
--
sqlite3_reset => noop
SECONDRESET: sqlite3_reset => noop
sqlite3_reset => noop
--
--
sqlite3_reset => noop
SECONDRESET: sqlite3_reset => noop
sqlite3_reset => noop
--
--
sqlite3_reset => noop
SECONDRESET: sqlite3_reset => noop
sqlite3_reset => noop
--
--
sqlite3_reset => noop
SECONDRESET: sqlite3_reset => noop
sqlite3_reset => noop
--
--
sqlite3_reset => noop
SECONDRESET: sqlite3_reset => noop
sqlite3_reset => noop
--
--
sqlite3_reset => noop
SECONDRESET: sqlite3_reset => noop
sqlite3_reset => noop
--
--
sqlite3_reset => noop
SECONDRESET: sqlite3_reset => noop

--

___
Python tracker 

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

[issue43350] [sqlite3] Active statements are reset twice in _pysqlite_query_execute()

[Erlend E. Aasland]

Erlend E. Aasland  added the comment:

Tests that exercise this branch:
Lib/sqlite3/test/dbapi.py: test_in_transaction
Lib/sqlite3/test/dbapi.py: test_last_row_id_insert_o_r
Lib/sqlite3/test/dbapi.py: 
test_on_conflict_abort_raises_with_explicit_transactions
Lib/sqlite3/test/dbapi.py: test_on_conflict_abort_raises_without_transactions
Lib/sqlite3/test/dbapi.py: test_on_conflict_rollback_with_explicit_transaction
Lib/sqlite3/test/dbapi.py: test_on_conflict_rollback_without_transaction
Lib/sqlite3/test/types.py: test_cursor_description_cte
Lib/sqlite3/test/types.py: test_bool
Lib/sqlite3/test/types.py: test_error_in_conform
Lib/sqlite3/test/userfunctions.py: test_aggr_check_aggr_sum
Lib/sqlite3/test/regression.py: test_column_name_with_spaces
Lib/sqlite3/test/regression.py: test_statement_reset

--

___
Python tracker 

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

[issue43350] [sqlite3] Active statements are reset twice in _pysqlite_query_execute()

[Erlend E. Aasland]

Erlend E. Aasland  added the comment:

Relevant historical commits:
- 
https://github.com/ghaering/pysqlite/commit/a471f0495956c3b8e3f45895b172e522a9ecd683
- 
https://github.com/ghaering/pysqlite/commit/5a009ed6fb2e90b952438f5786f93cd1e8ac8722
- 191321d11bc7e064e1a07830a43fa600de310e1b (in cpython repo)

--

___
Python tracker 

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

[issue43350] [sqlite3] Active statements are reset twice in _pysqlite_query_execute()

[Erlend E. Aasland]

Erlend E. Aasland  added the comment:

Relevant historical commits:
- 
https://github.com/ghaering/pysqlite/commit/a471f0495956c3b8e3f45895b172e522a9ecd683
- 
https://github.com/ghaering/pysqlite/commit/5a009ed6fb2e90b952438f5786f93cd1e8ac8722
- 191321d11bc7e064e1a07830a43fa600de310e1bj (in cpython repo)

--

___
Python tracker 

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

[issue43350] [sqlite3] Active statements are reset twice in _pysqlite_query_execute()

[Erlend E. Aasland]

Change by Erlend E. Aasland :


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

___
Python tracker 

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

[issue43350] [sqlite3] Active statements are reset twice in _pysqlite_query_execute()

[Greg Stein]

Change by Greg Stein :


--
nosy: +gstein
nosy_count: 3.0 -> 4.0
pull_requests: +24667
pull_request: https://github.com/python/cpython/pull/26015

___
Python tracker 

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

[issue43350] [sqlite3] Active statements are reset twice in _pysqlite_query_execute()

[Berker Peksag]

Berker Peksag  added the comment:

Serhiy is right. Without a proper research on why it was added in the first 
place, simply removing the second call won't make code any better and it may 
introduce regressions (we've already introduced two in 3.10) Maybe doing some 
digging in pysqlite commits or reaching out to the original author may help.

--

___
Python tracker 

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

[issue43350] [sqlite3] Active statements are reset twice in _pysqlite_query_execute()

[Erlend E. Aasland]

Erlend E. Aasland  added the comment:

> Hm, I guess we could use sqlite3_stmt_busy() instead of the in_use flag.

I opened bpo-44073 for this.

--

___
Python tracker 

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

[issue43350] [sqlite3] Active statements are reset twice in _pysqlite_query_execute()

[Erlend Egeberg Aasland]

Erlend Egeberg Aasland  added the comment:

Hm, I guess we could use sqlite3_stmt_busy() instead of the in_use flag.

--

___
Python tracker 

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

[issue43350] [sqlite3] Active statements are reset twice in _pysqlite_query_execute()

[Erlend Egeberg Aasland]

Erlend Egeberg Aasland  added the comment:

> There are three calls of pysqlite_statement_reset() in 
> _pysqlite_query_execute()

Yes, but only two before the cache is queried.

> So additional call of pysqlite_statement_reset() does not harm.

That's true, but removing redundant code makes it easier to read and maintain, 
IMO.

> On other hand removing it can introduce race condition.

How?

> Maybe the code could be rewritten in more explicit way and call 
> pysqlite_statement_reset() only when it is necessary

Most of _pysqlite_query_execute() could be rewritten in order to make the code 
clearer. An alternative is to clean it up part by part.

--

___
Python tracker 

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

[issue43350] [sqlite3] Active statements are reset twice in _pysqlite_query_execute()

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

There are three calls of pysqlite_statement_reset() in 
_pysqlite_query_execute(). And all three of them can be called with the same 
statement object. But repeated calls are no-ops because 
pysqlite_statement_reset() clears flag in_use and calls sqlite3_reset() only if 
it was set before. So additional call of pysqlite_statement_reset() does not 
harm. You can call it as many times as you want. On other hand removing it can 
introduce race condition.

Maybe the code could be rewritten in more explicit way and call 
pysqlite_statement_reset() only when it is necessary, but for now I don't think 
that just removing one call will make the code better.

--

___
Python tracker 

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

[issue43350] [sqlite3] Active statements are reset twice in _pysqlite_query_execute()

[Erlend Egeberg Aasland]

Change by Erlend Egeberg Aasland :


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

___
Python tracker 

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

[issue43350] [sqlite3] Active statements are reset twice in _pysqlite_query_execute()

[Erlend Egeberg Aasland]

New submission from Erlend Egeberg Aasland :

In _pysqlite_query_execute(), if the cursor already has a statement object, it 
is reset twice before the cache is queried.

--
components: Library (Lib)
messages: 387850
nosy: berker.peksag, erlendaasland, serhiy.storchaka
priority: normal
severity: normal
status: open
title: [sqlite3] Active statements are reset twice in _pysqlite_query_execute()
type: enhancement
versions: Python 3.10

___
Python tracker 

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