[issue44079] [sqlite3] remove superfluous statement weak ref list from connection object

[Erlend E. Aasland]

Erlend E. Aasland  added the comment:

Thanks Pablo for merging, and Berker for pushing me to investigate further. 
Highly appreciated!

--

___
Python tracker 

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

[issue44079] [sqlite3] remove superfluous statement weak ref list from connection object

[Pablo Galindo Salgado]

Change by Pablo Galindo Salgado :


--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

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

[issue44079] [sqlite3] remove superfluous statement weak ref list from connection object

[Pablo Galindo Salgado]

Pablo Galindo Salgado  added the comment:


New changeset 243b6c3b8fd3144450c477d99f01e31e7c3ebc0f by Erlend Egeberg 
Aasland in branch 'main':
bpo-44079: Strip superfluous statement cache from sqlite3.Connection (GH-25998)
https://github.com/python/cpython/commit/243b6c3b8fd3144450c477d99f01e31e7c3ebc0f


--
nosy: +pablogsal
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

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

[issue44079] [sqlite3] remove superfluous statement weak ref list from connection object

[Erlend E. Aasland]

Erlend E. Aasland  added the comment:

Berker:
> My understanding is that their usages is a bit different:
> one is for performance reasons (and it can configurable by users) and the 
> other is to keep track of statements inside a connection.
>
> I'm not fully sure that the both use cases can be combined.

Only unique statements are added to the weak ref list (and the LRU cache).

Case 1: len(unique statements) <= 128
Both the LRU cache and the weak ref list will be equal sets of statements.

Case 2: len(unique statements) > 128 <= 200
If more than 128 unique statements are created, they "fall off" of the LRU 
cache and are collected by the GC. The weak ref list will contain all the 128 
cached statements, and up to 72 "lost" statements which have been reaped by the 
GC.
The LRU cache and the _alive_ objects in weak ref list will be equal sets of 
statements.

Case 3: len(unique statements) > 200
_pysqlite_drop_unused_statement_references() will remove the lost 72 refs and 
create a new weak ref list that will be equal to the 128 statements in the LRU 
cache.

Fun fact: After the number of unique statements pass 200, the weak ref list is 
reset to the remaining 128 "active" statements, and 
sqlite3.Connection.created_statements is reset to 0. Thus, when we pass the 
"200 limit" again, the weak ref list will contain 329 elements, not 200 
elements.

Any statement that "falls off" of the LRU cache is reaped by the GC which 
implies that sqlite3_finalize() is called on it, and it's thus removed from the 
SQLite connection. This implies that the set of statements available through 
sqlite3_next_stmt() is equal to the set of statements in the LRU cache and the 
set of _alive_ statements in the weak ref list.

This implies that if we want to finalise all connection statements in 
sqlite3.Connection.close(), all we need to do is to clear the LRU cache; the GC 
will invoke stmt_dealloc(), which will finalise the SQLite statement objects. 
If we want to reset all connection statements (pysqlite_do_all_statements()), 
we can iterate through them using sqlite3_next_stmt().

However, there is a corner case for when the sqlite3.Connection.statements list 
can contain statements that are not in the LRU cache: if we've got more than 
128 active cursors, all of them with unique statements, we can end up with 
statements that are _only_ owned by a cursor. However, sqlite3_next_stmt() will 
still find them if we need to reset them in pysqlite_do_all_statements(). On 
connection close, all cursors are destroyed anyway, so we will end up 
finalising all statements, weak ref list or not.

Can we merge PR-25998 now?

Prove me wrong :)

--

___
Python tracker 

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

[issue44079] [sqlite3] remove superfluous statement weak ref list from connection object

[Erlend E. Aasland]

Erlend E. Aasland  added the comment:

Also, note that if identical SQL statements are created in multiple cursors 
belonging to the same connection, they will currently _not_ be added to the 
connection weak ref list. See the if (self->statement->in_use) in the middle of 
_pysqlite_query_execute(). Thus, the current code actually fails to register 
_all_ statements in the "all statements" weak ref list. IMO, this is yet 
another argument to abandon that list.

Side note: This branch is not exercised in the unit test suite. Adding the 
following test will cover this branch:

diff --git a/Lib/sqlite3/test/dbapi.py b/Lib/sqlite3/test/dbapi.py
index c17f911fa1..8e53c657a6 100644
--- a/Lib/sqlite3/test/dbapi.py
+++ b/Lib/sqlite3/test/dbapi.py
@@ -526,6 +526,10 @@ def test_last_row_id_insert_o_r(self):
 ]
 self.assertEqual(results, expected)
 
+def test_same_query_in_multiple_cursors(self):
+cursors = [self.cx.execute("select 1") for _ in range(3)]
+for cu in cursors:
+self.assertEqual(cu.fetchall(), [(1,)])
 
 class ThreadTests(unittest.TestCase):
 def setUp(self):


See bpo-43553 for sqlite3 coverage improvements.

--

___
Python tracker 

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

[issue44079] [sqlite3] remove superfluous statement weak ref list from connection object

[Erlend E. Aasland]

Erlend E. Aasland  added the comment:

> I'm not fully sure that the both use cases can be combined.

I'm fully sure they can :) Did I miss anything in msg393942? AFAICS, there is 
no need for the weak ref list anymore.

--

___
Python tracker 

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

[issue44079] [sqlite3] remove superfluous statement weak ref list from connection object

[Erlend E. Aasland]

Erlend E. Aasland  added the comment:

Yes, that seems to be the intention. But, I don't think there is a need to 
maintain the second list:

1. Resetting statements was historically needed both for commit and rollback; 
pending statements would block such operations. That's no longer the case for 
recent SQLite versions (commits fixed in SQLite 3.6.5, rollbacks in SQLite 
3.7.11). The sqlite3 module no longer reset commit statements 
(6ed442c48dd7f8d3097e688a36bc027df3271621), and there should be no need to 
reset rollbacks either (see bpo-44092).
2. A statement with ref count zero will be garbage collected. Thus, of a 
statement is dropped from the LRU cache, it will be sqlite3_finalize'd. No 
statement, no problem. It's the same effect as if 
_pysqlite_drop_unused_statement_references() used the same limit as the LRU 
cache.

If we want to explicit reset statements (for some reason), we'd be better off 
to use sqlite3_stmt_next(). It's faster, and it covers _all_ statements for a 
given connection.

--

___
Python tracker 

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

[issue44079] [sqlite3] remove superfluous statement weak ref list from connection object

[Berker Peksag]

Berker Peksag  added the comment:

https://github.com/ghaering/pysqlite/commit/33f99be6be5ad7d69767ff172441f1a860416e82
 states the following:

- Use a list of weak references of statements in the Connection class to 
keep
  track of all statements used with this connection. This is necessary to be
  able to reset all statements used in a connection.

My understanding is that their usages is a bit different: one is for 
performance reasons (and it can configurable by users) and the other is to keep 
track of statements inside a connection.

I'm not fully sure that the both use cases can be combined.

--
type: resource usage -> enhancement

___
Python tracker 

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

[issue44079] [sqlite3] remove superfluous statement weak ref list from connection object

[Erlend E. Aasland]

Change by Erlend E. Aasland :


--
title: [sqlite3] optimisation: remove statement weak ref list from connection 
object -> [sqlite3] remove superfluous statement weak ref list from connection 
object

___
Python tracker 

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