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

@berkerpeksag

BPO	44079
Nosy	@berkerpeksag, @serhiy-storchaka, @pablogsal, @erlend-aasland
PRs	bpo-44079: Strip superfluous statement cache from sqlite3.Connection #25998

^{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 = 'https://github.com/erlend-aasland'
closed_at = <Date 2021-08-18.23:38:02.412>
created_at = <Date 2021-05-08.19:43:24.263>
labels = ['extension-modules', 'type-feature', '3.11']
title = '[sqlite3] remove superfluous statement weak ref list from connection object'
updated_at = <Date 2021-08-19.07:03:39.684>
user = 'https://github.com/erlend-aasland'

bugs.python.org fields:

activity = <Date 2021-08-19.07:03:39.684>
actor = 'erlendaasland'
assignee = 'erlendaasland'
closed = True
closed_date = <Date 2021-08-18.23:38:02.412>
closer = 'pablogsal'
components = ['Extension Modules']
creation = <Date 2021-05-08.19:43:24.263>
creator = 'erlendaasland'
dependencies = []
files = []
hgrepos = []
issue_num = 44079
keywords = ['patch']
message_count = 9.0
messages = ['393282', '393283', '393936', '393942', '394269', '394848', '399559', '399875', '399888']
nosy_count = 4.0
nosy_names = ['berker.peksag', 'serhiy.storchaka', 'pablogsal', 'erlendaasland']
pr_nums = ['25998']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue44079'
versions = ['Python 3.11']

Today, the sqlite3 extension module keeps two[1] lists of statements:

The statement cache[2], stored in pysqlite_Connection.statement_cache
A weak ref. list (hard coded limited to 200 statements in _pysqlite_drop_unused_statement_references()), stored in pysqlite_Connection.statements

AFAICS, there's no benefit in keeping both. Suggesting to remove the latter.

Pro's:

less lines of code to maintain
clearer code
one less data member in pysqlite_Connection
improved test coverage (there are currently no coverage for _pysqlite_drop_unused_statement_references()[3])

Con's:

the current code works / "code churn"

$ git diff --shortstat 
 1 file changed, 13 insertions(+), 60 deletions(-)

[1] Actually, there's three lists: SQLite also keeps a list, available using sqlite3_stmt_next()
[2] See bpo-42862 for suggested improvements to the current cache implementation
[3] See bpo-43553 for improving the code coverage of the sqlite3 extension module

Berker/Serhiy, weak ref. list question:
Could also the pysqlite_Statement.in_weakreflist member go? Seems to be ok:

$ git diff --shortstat
 3 files changed, 13 insertions(+), 72 deletions(-)
$ ./python.exe -m test -R : test_sqlite
0:00:00 load avg: 0.83 Run tests sequentially
0:00:00 load avg: 0.83 [1/1] test_sqlite
beginning 9 repetitions
123456789
.........

== Tests result: SUCCESS ==

1 test OK.

Total duration: 5.7 sec
Tests result: SUCCESS

ghaering/pysqlite@33f99be 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.

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

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 (6ed442c), and there should be no need to reset rollbacks either (see bpo-44092).
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.

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.

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.

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

New changeset 243b6c3 by Erlend Egeberg Aasland in branch 'main':
bpo-44079: Strip superfluous statement cache from sqlite3.Connection (GH-25998)
243b6c3

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

Navigation Menu

Repository navigation

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

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

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

Description

Activity

erlend-aasland commented on May 8, 2021

erlend-aasland commented on May 8, 2021

berkerpeksag commented on May 19, 2021

erlend-aasland commented on May 19, 2021

erlend-aasland commented on May 24, 2021

erlend-aasland commented on Jun 1, 2021

erlend-aasland commented on Aug 13, 2021

pablogsal commented on Aug 18, 2021

erlend-aasland commented on Aug 19, 2021

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions