Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document sqlite3.connect() as implicitly opening transactions in the new PEP-249 manual commit mode #99824

Closed
geryogam opened this issue Nov 27, 2022 · 5 comments
Labels
docs Documentation in the Doc dir topic-sqlite3

Comments

@geryogam
Copy link
Contributor

geryogam commented Nov 27, 2022

Documentation

In sqlite3, @malemburg specified that connect(), commit(), and rollback() implicitly open transactions in the new PEP-249 manual commit mode implemented in PR #93823:

If this is set to False, the module could then implement the
correct way of handling transactions, which means:

a) start a new transaction when the connection is opened
b) start a new transaction after .commit() and .rollback()
c) don't start new transactions anywhere else
d) run an implicit .rollback() when the connection closes

So I expect to see that information clearly documented.

Yet the PR documented it only for commit() and rollback() (item b), not for connect() (item a):

* :mod:`!sqlite3` ensures that a transaction is always open,
  so :meth:`Connection.commit` and :meth:`Connection.rollback`
  will implicitly open a new transaction immediately after closing
  the pending one.

To me it is important to document it also for connect() rather than relying on user’s deduction, which is not obvious at all since for instance in legacy manual commit mode, sqlite3 does not implicitly open transactions when connect() is called but when execute() is called on a DML statement.

Linked PRs

@erlend-aasland
Copy link
Contributor

Thanks for creating an issue and explaining your reasoning behind this proposed change.

In sqlite3, @malemburg specified [...]

For your information; the PEP 249 is the specification, and Marc-André has recently updated it with information about the autocommit attribute:

https://peps.python.org/pep-0249/#autocommit

@erlend-aasland
Copy link
Contributor

erlend-aasland commented Nov 27, 2022

[...] Yet the PR documented it only for commit() and rollback() (item b), not for connect() (item a):

* :mod:`!sqlite3` ensures that a transaction is always open,
  so :meth:`Connection.commit` and :meth:`Connection.rollback`
  will implicitly open a new transaction immediately after closing
  the pending one.

To me it is important to document it also for connect() rather than relying on user’s deduction for connect(), which is not obvious at all since for instance in legacy manual commit mode, sqlite3 does not implicitly open transactions when connect() is called but when execute() is called on a DML statement.

The docs you point to are the docs explaining transaction control using the autocommit attribute. This section is only relevant if you've got a Connection object, so I'm not sure this is the best place (in the docs) to add a sentence about connect() behaviour. If we want to explain the behaviour of sqlite3.connect's autocommit parameter, we should do so in that part of the reference1.

Footnotes

  1. https://docs.python.org/3/library/sqlite3.html#sqlite3.connect

@geryogam
Copy link
Contributor Author

geryogam commented Nov 28, 2022

Yes we should probably document the implicit transaction opening behaviour of the connect() function in Reference > Module functions > sqlite3.connect, like we did for the Connection.commit() and Connection.rollback() methods in Reference > Connection objects > sqlite3.Connection.commit and Reference > Connection objects > sqlite3.Connection.rollback:

commit()

Commit any pending transaction to the database. If autocommit is True, or there is no open transaction, this method does nothing. If autocommit is False, a new transaction is implicitly opened if a pending transaction was committed by this method.

rollback()

Roll back to the start of any pending transaction. If autocommit is True, or there is no open transaction, this method does nothing. If autocommit is False, a new transaction is implicitly opened if a pending transaction was rolled back by this method.

But we should also probably document it in Explanation > Transaction control since it is a general section about transaction control. And as you said, when the autocommit parameter is False a transaction is implicitly opened for connect(), Connection.commit() and Connection.rollback(), whereas when the Connection.autocommit attribute is False a transaction is implicitly opened only for Connection.commit() and Connection.rollback() since the Connection instance is already created. So the autocommit parameter and the Connection.autocommit attribute are actually two different ways of controlling transactions. What about renaming the ‘Transaction control via the autocommit attribute’ subsection to ‘Transaction control via the autocommit parameter or attribute’, or simply to ‘Transaction control via autocommit’. Likewise for the ‘Transaction control via the isolation_level attribute’ subsection?

@erlend-aasland
Copy link
Contributor

But we should also probably document it in Explanation > Transaction control since it is a general section about transaction control.

I'm -1 for that change; I'm afraid you are overcomplicating things. We should keep the docs (both reference and explanation, but especially the former) to the point; we should resist the urge to be overly verbose.

IMO, the right thing to do, is to update the sqlite3.connect() reference only.

So the autocommit parameter and the Connection.autocommit attribute are actually two different ways of controlling transactions [...]

No, they are the same; the connect autocommit parameter only provides a way to set the initial value of the Connection.autocommit attribute.

What about renaming the ‘Transaction control via the autocommit attribute’ subsection to ‘Transaction control via the autocommit parameter or attribute’, or simply to ‘Transaction control via autocommit’. Likewise for the ‘Transaction control via the isolation_level attribute’ subsection?

I'm -1 for such a change; the initial value of both attributes can be set using a keyword parameter when the connection object is set up. This is all explained in the reference; please take a look at the existing docs. There is no need to be overly verbose.

@geryogam
Copy link
Contributor Author

IMO, the right thing to do, is to update the sqlite3.connect() reference only.

Done.

erlend-aasland pushed a commit that referenced this issue Nov 30, 2022
… if autocommit=False (#99825)

Authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
carljm added a commit to carljm/cpython that referenced this issue Dec 1, 2022
* main: (112 commits)
  pythongh-99894: Ensure the local names don't collide with the test file in traceback suggestion error checking (python#99895)
  pythongh-99612: Fix PyUnicode_DecodeUTF8Stateful() for ASCII-only data (pythonGH-99613)
  Doc: Add summary line to isolation_level & autocommit sqlite3.connect params (python#99917)
  pythonGH-98906 ```re``` module: ```search() vs. match()``` section should mention ```fullmatch()``` (pythonGH-98916)
  pythongh-89189: More compact range iterator (pythonGH-27986)
  bpo-47220: Document the optional callback parameter of weakref.WeakMethod (pythonGH-25491)
  pythonGH-99905: Fix output of misses in summarize_stats.py execution counts (pythonGH-99906)
  pythongh-99845: PEP 670: Convert PyObject macros to functions (python#99850)
  pythongh-99845: Use size_t type in __sizeof__() methods (python#99846)
  pythonGH-99877)
  Fix typo in exception message in `multiprocessing.pool` (python#99900)
  pythongh-87092: move all localsplus preparation into separate function called from assembler stage (pythonGH-99869)
  pythongh-99891: Fix infinite recursion in the tokenizer when showing warnings (pythonGH-99893)
  pythongh-99824: Document that sqlite3.connect implicitly open a transaction if autocommit=False (python#99825)
  pythonGH-81057: remove static state from suggestions.c (python#99411)
  Improve zip64 limit error message (python#95892)
  pythongh-98253: Break potential reference cycles in external code worsened by typing.py lru_cache (python#98591)
  pythongh-99127: Allow some features of syslog to the main interpreter only (pythongh-99128)
  pythongh-82836: fix private network check (python#97733)
  Docs: improve accuracy of socketserver reference (python#24767)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir topic-sqlite3
Projects
Status: Done
Development

No branches or pull requests

2 participants