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

gh-128505: Expose an interface to sqlite3_file_control #128507

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

hashbrowncipher
Copy link
Contributor

@hashbrowncipher hashbrowncipher commented Jan 5, 2025

sqlite3_file_control can affect the semantics of a sqlite3 database, but is not exposed by the sqlite.Connection class. In particular, this allows callers to use the SQLITE_FCNTL_PERSIST_WAL opcode. SQLITE_FCNTL_PERSIST_WAL ensures that callers can open a WAL-mode database on a read-only mount even when the writers have closed the DB.

Copy link

cpython-cla-bot bot commented Jan 5, 2025

All commit authors signed the Contributor License Agreement.
CLA signed

@erlend-aasland erlend-aasland self-assigned this Jan 5, 2025
Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I briefly started looking at this; will do a full review later today (CET).

Lib/test/test_sqlite3/test_dbapi.py Outdated Show resolved Hide resolved
Lib/test/test_sqlite3/test_dbapi.py Outdated Show resolved Hide resolved
@@ -0,0 +1 @@
sqlite Connection objects now expose a method set_file_control, which is a thin wrapper for `sqlite3_file_control https://www.sqlite.org/c3ref/file_control.html`_.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See https://devguide.python.org/documentation/markup/ for help with docs markup.

Modules/_sqlite/connection.c Outdated Show resolved Hide resolved
Modules/_sqlite/connection.c Outdated Show resolved Hide resolved
Modules/_sqlite/connection.c Outdated Show resolved Hide resolved
/*[clinic end generated code: output=d9d2d311892893b6 input=0253798d9514fea2]*/
{
int rc;
long val = arg;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not all sqlite3_file_control opcodes use the int1 datatype. For example SQLITE_FCNTL_VFSNAME and SQLITE_FCNTL_TEMPFILENAME are char *. Suggesting to limit this (like we do for sqlite3_db_config) to only the supported opcodes, and raise an exception for unsupported opcodes.

In any case, we should use int here.

Suggested change
long val = arg;
int val = arg;

Footnotes

  1. int, not long

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also address the first part of this remark.

Py_END_ALLOW_THREADS

if (rc != SQLITE_OK) {
PyErr_SetString(self->ProgrammingError, sqlite3_errstr(rc));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ProgrammingError may not be the right fit for all opcodes. Ideally, we should use a variant of _pysqlite_seterror where we pass an error code instead of a SQLite database pointer. Perhaps I should make such an internal API available first.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be happy to use any internal API that you build for this purpose.

@erlend-aasland
Copy link
Contributor

Just a heads-up: we normally do not force-push. PRs are squashed into a single commit when we merge to main anyway, so there is no need to try and keep a nice Git commit history. See also the devguide.


op: int
The SQLITE_FCNTL_* constant to invoke.
arg: long
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's stick with int, since that's the datatype used by SQLite for these opcodes.

Suggested change
arg: long
arg: int

Comment on lines +1 to +3
sqlite Connection objects now expose a method
:meth:`sqlite3.Connection.file_control`, which is a thin wrapper for
`sqlite3_file_control <https://www.sqlite.org/c3ref/file_control.html>`_.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also need to add docs for file_control. See Doc/library/sqlite3.rst.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants