[issue45126] [sqlite3] cleanup and harden connection init

[Erlend E. Aasland]

Erlend E. Aasland  added the comment:

I'll save the cleanup till Python 3.13 dev is started. I've opened a PR for 
fixing the ref leak (should be backported), and a draft PR for deprecating 
Connection and Cursor reinitialisation.

--

___
Python tracker 

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

[issue45126] [sqlite3] cleanup and harden connection init

[Erlend E. Aasland]

Change by Erlend E. Aasland :


--
pull_requests: +26654
pull_request: https://github.com/python/cpython/pull/28234

___
Python tracker 

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

[issue45126] [sqlite3] cleanup and harden connection init

[Erlend E. Aasland]

Change by Erlend E. Aasland :


--
pull_requests: +26651
pull_request: https://github.com/python/cpython/pull/28231

___
Python tracker 

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

[issue45126] [sqlite3] cleanup and harden connection init

[Erlend E. Aasland]

Erlend E. Aasland  added the comment:

> If we deprecate but keep the buggy behavior it as it is, (1) is not needed. 
> Less work for both us and the users.

Indeed.

There's still a ref leak I'd like to take care of, though: if the first audit 
fails, database_obj leaks.

--

___
Python tracker 

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

[issue45126] [sqlite3] cleanup and harden connection init

[Petr Viktorin]

Petr Viktorin  added the comment:

As for the effort to fix this:

If we deprecate this, there should be no new users of it in 3.11+.

If we deprecate and also fix this, and we happen to introduce bugs or behavior 
changes, then people that use it now will need to:
1) adapt to the new behavior
2) find a different way to do things (before 3.13)

If we deprecate but keep the buggy behavior it as it is, (1) is not needed. 
Less work for both us and the users.

--

___
Python tracker 

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

[issue45126] [sqlite3] cleanup and harden connection init

[Erlend E. Aasland]

Erlend E. Aasland  added the comment:

Great, thanks.

--

___
Python tracker 

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

[issue45126] [sqlite3] cleanup and harden connection init

[Petr Viktorin]

Petr Viktorin  added the comment:

I think a deprecation should be discussed a bit more widely than on bpo, so I 
opened a thread here: 
https://discuss.python.org/t/deprecating-sqlite-object-reinitialization/10503

--

___
Python tracker 

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

[issue45126] [sqlite3] cleanup and harden connection init

[Erlend E. Aasland]

Erlend E. Aasland  added the comment:

FYI, I've expanded the reinit tests and added a deprecation warning to the PR.

--

___
Python tracker 

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

[issue45126] [sqlite3] cleanup and harden connection init

[Erlend E. Aasland]

Erlend E. Aasland  added the comment:

Modifying the loops to also print the values:

first fetch
b'0' 
b'1' 
second fetch
2 
3 

--

___
Python tracker 

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

[issue45126] [sqlite3] cleanup and harden connection init

[Erlend E. Aasland]

Erlend E. Aasland  added the comment:

Note: I ran that with PR 28227.


OTOH: I do agree that there's a lot of pitfalls here, especially in the future. 
In the long run, it is probably best to deprecate reinit, and disable it in 
Python 3.13.

--

___
Python tracker 

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

[issue45126] [sqlite3] cleanup and harden connection init

[Erlend E. Aasland]

Erlend E. Aasland  added the comment:

I modified your second example slightly:

```
import sqlite3

conn = sqlite3.connect(":memory:")
conn.text_factory=bytes
conn.row_factory = sqlite3.Row
cursor = conn.execute("CREATE TABLE foo (bar)")
numbers = range(4)
cursor.executemany("INSERT INTO foo (bar) VALUES (?)", ((str(v),) for v in 
numbers))
cursor.execute("SELECT bar FROM foo")

print("first fetch")
for row in cursor.fetchmany(2):
print(type(row[0]))

conn.__init__(":memory:")
conn.execute("CREATE TABLE foo (bar)")
letters = "a", "b", "c", "d"
conn.executemany("INSERT INTO foo (bar) VALUES (?)", ((v,) for v in letters))

# Currently this uses the old database, old row_factory, but new text_factory"
print("second fetch")
for row in cursor.fetchall():
print(type(row[0]))
```

Here's the output:
first fetch


second fetch



--

___
Python tracker 

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

[issue45126] [sqlite3] cleanup and harden connection init

[Petr Viktorin]

Petr Viktorin  added the comment:

This is a minefield. If anyone has a use case for it, I'd *love* to hear it, 
but to me it seems that proper reinit support will be a lot of work (now and in 
future maintenance) for no gain. You can always create a new connection object.

Consider instead deprecating reinitialization, documenting it as unpredictable 
& removing it in Python 3.13.


Here's another thing to test:
```python
import sqlite3

conn = sqlite3.connect(':memory:')
cursor = conn.execute('CREATE TABLE foo (bar)')

try:
conn.__init__('/bad-file/')
except sqlite3.OperationalError:
pass

cursor.execute('INSERT INTO foo (bar) VALUES (1), (2), (3), (4)')
```

And here's the kind of behavior that should be specified (and tested), and 
might even change to make more sense:
```python
import sqlite3

conn = sqlite3.connect(':memory:')
conn.text_factory=bytes
conn.row_factory = sqlite3.Row
cursor = conn.execute('CREATE TABLE foo (bar)')
cursor.execute('INSERT INTO foo (bar) VALUES ("1"), ("2"), ("3"), ("4")')
cursor.execute('SELECT bar FROM foo')

for row in cursor:
print(row, list(row))
break

conn.__init__(':memory:')
conn.execute('CREATE TABLE foo (bar)')
conn.execute('INSERT INTO foo (bar) VALUES ("a"), ("b"), ("c"), ("d")')

# Currently this uses the old database, old row_factory, but new text_factory"
for row in cursor:
print(row, list(row))
```

We should also do a review of all places `connection` is used from Cursor, 
Statement, etc. to ensure everything continue to work with a different `db`. 
This can be done (and maybe you already did), but I'm also worried how well 
future maintainers can keep reinitialization in mind.

And all this will also need to be done for Cursor.

--

___
Python tracker 

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

[issue45126] [sqlite3] cleanup and harden connection init

[Erlend E. Aasland]

Change by Erlend E. Aasland :


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

___
Python tracker 

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

[issue45126] [sqlite3] cleanup and harden connection init

[Erlend E. Aasland]

New submission from Erlend E. Aasland :

Quoting Petr Viktorin in PR 27940, 
https://github.com/python/cpython/pull/27940#discussion_r703424148:

- db is not set to NULL if init fails.
- This segfaults for me:

  import sqlite3

  conn = sqlite3.connect(':memory:')
  conn.execute('CREATE TABLE foo (bar)')

  try:
  conn.__init__('/bad-file/')
  except sqlite3.OperationalError:
  pass

  conn.execute('INSERT INTO foo (bar) VALUES (1), (2), (3), (4)')


Other issues:
- reinitialisation is not handled gracefully
- __init__ is messy; members are initialised here and there


Suggested to reorder connection __init__ in logical groups to more easily 
handle errors:
  1. handle reinit
  2. open and configure database
  3. create statement LRU cache and weak ref cursor list
  4. initialise members in the order they're defined in connection.h
  5. set isolation level, since it's a weird case anyway

--
components: Extension Modules
messages: 401248
nosy: erlendaasland, petr.viktorin
priority: normal
severity: normal
status: open
title: [sqlite3] cleanup and harden connection init
versions: Python 3.11

___
Python tracker 

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