bpo-9303: Migrate sqlite3 module to _v2 API to enhance performance by palaviv · Pull Request #359 · python/cpython (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation17 Commits10 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Use the _v2 API when possible.

Choose a reason for hiding this comment

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

I think we can also avoid calling sqlite3_reset in _pysqlite_seterror if the v2 API is available.

I haven't checked yet, but I also think that the check for SQLITE_SCHEMA can also be eliminated if the v2 API is available:

if (rc == SQLITE_SCHEMA) {

@@ -118,8 +118,13 @@ int pysqlite_connection_init(pysqlite_Connection* self, PyObject* args, PyObject
return -1;
}
Py_BEGIN_ALLOW_THREADS
#ifdef HAVE_SQLITE3_OPEN_V2
rc = sqlite3_open_v2(database, &self->db,
SQLITE_OPEN_READWRITE \| SQLITE_OPEN_CREATE, NULL);

Choose a reason for hiding this comment

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

Style nitpick: FOO|BAR is more common than FOO | BAR in sqlite3 code base.

@@ -118,8 +118,13 @@ int pysqlite_connection_init(pysqlite_Connection* self, PyObject* args, PyObject
return -1;
}
Py_BEGIN_ALLOW_THREADS
#ifdef HAVE_SQLITE3_OPEN_V2
rc = sqlite3_open_v2(database, &self->db,

Choose a reason for hiding this comment

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

I think that there is an opportunity to simplify this whole SQLITE_OPEN_URI and HAVE_SQLITE3_OPEN_V2 dance.

(The use of Py_BEGIN_ALLOW_THREADS is a bit weird in line 111 and line 120.)

I think we can also avoid calling sqlite3_reset in _pysqlite_seterror if the v2 API is available.

done

I haven't checked yet, but I also think that the check for SQLITE_SCHEMA can also be eliminated if the v2 API is available:

if (rc == SQLITE_SCHEMA) {

I don't think we should do this because we can get SQLITE_SCHEMA if SQLITE_MAX_SCHEMA_RETRY tries to recompile will pass.

Choose a reason for hiding this comment

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

This looks pretty good to me. We just need an entry in Misc/NEWS (please add "Patch by Aviv Palivoda." too)

if (st != NULL) {
(void)sqlite3_reset(st);
}
#endif

Choose a reason for hiding this comment

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

Nitpick: Please remove extra empty line.

Choose a reason for hiding this comment

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

done

@@ -118,6 +118,8 @@ int pysqlite_connection_init(pysqlite_Connection* self, PyObject* args, PyObject
return -1;
}
Py_BEGIN_ALLOW_THREADS
/* No need to use sqlite3_open_v2 as sqlite3_open(filename, db) is the
same as sqlite3_open_v2(filename, db, SQLITE_OPEN_READWRITE \| SQLITE_OPEN_CREATE, NULL)*/

Choose a reason for hiding this comment

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

Nitpick: FOO|BAR syntax instead of FOO | BAR.

Also, finish the line with a full stop and space:

Choose a reason for hiding this comment

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

done

I don't think we should do this because we can get SQLITE_SCHEMA if SQLITE_MAX_SCHEMA_RETRY tries to recompile will pass.

Right, we can still get SQLITE_SCHEMA from sqlite3_step() (in legacy API we need to call sqlite3_reset() to get it), but in this case rc is the return value of sqlite3_reset(). The latter call is not needed when we have the new API since sqlite3_step() calls sqlite3_reset() for us.

I have removed the reset but I left the recompile because we should recompile before calling sqlite3_step again.