classification
Title: SQLite trace callback
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: ghaering, pitrou, python-dev, torsten
Priority: normal Keywords: patch

Created on 2011-03-26 22:09 by torsten, last changed 2011-04-04 00:22 by pitrou. This issue is now closed.

Files
File name Uploaded Description Edit
sqlite_trace_py3.diff torsten, 2011-03-26 22:09 Patch for default branch review
sqlite_trace_py27.diff torsten, 2011-03-26 22:11 Patch for Python 2.7 review
sqlite_trace.diff torsten, 2011-03-29 23:02 Updated patch for default branch review
Messages (14)
msg132275 - (view) Author: Torsten Landschoff (torsten) Date: 2011-03-26 22:09
I'd like to access the SQLite trace callback from Python to actually see the same queries that SQLite actually gets to see.

The C API of SQLite supports this via the sqlite3_trace function. I added support to this to the sqlite3 module plus unit tests testing that it is called, can be removed again and that unicode arguments arrive correctly at the callback.

Please consider applying the patch. I am not sure if this should go to the pysqlite project on Google Code directly, I am just submitting it here because I worked in the python source tree as I am more used to its build system. I am sorry, Gerhard, if this is the wrong way.
msg132276 - (view) Author: Torsten Landschoff (torsten) Date: 2011-03-26 22:11
Here is the same change for Python 2.7. I don't expect this to get merged on the Python 2 branch, put perhaps it is useful to somebody.
msg132332 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-03-27 14:31
Thanks for the patch.

A couple of comments:
- this is a new feature, so can only go in in 3.x: no need to post a 2.7 patch (unless this helps Gerhard for his standalone project)
- you need to document the new API in Doc/library/sqlite3.rst

About the patch: looks mostly good!

+        self.assertTrue([x for x in traced_statements if x.find("create table foo") != -1])

This looks a bit complicated, why not something like
`any("create table foo" in x for x in traced_statements)`?

(`y in x` is simper and more readable than `x.find(y) != -1`)


+        sqlite3_trace(self->db, _trace_callback, trace_callback);
+        if (PyDict_SetItem(self->function_pinboard, trace_callback, Py_None) == -1)
+            return NULL;

Shouldn't sqlite3_trace() be called only after PyDict_SetItem() succeeds?
msg132353 - (view) Author: Torsten Landschoff (torsten) Date: 2011-03-27 17:38
> A couple of comments:
> - this is a new feature, so can only go in in 3.x: no need to post a 2.7 patch (unless this helps Gerhard for his standalone project)

The motivation for the 2.7er patch is mostly that we are still using Python 2.x at work and I made this patch for 2.7 first. Actually I will need to backport it to 2.6 but I guess there are no differences. And maybe I should have submitted this against pysqlite directly which is (AFAICT) also still targetting 2.x.

> - you need to document the new API in Doc/library/sqlite3.rst

Sure. I was just filing the report to have the code on file and this was only fallout from another patch. I will create an updated patch including documentation about the feature.

> About the patch: looks mostly good!

Thanks.

> +        self.assertTrue([x for x in traced_statements if x.find("create table foo") != -1])
>
> This looks a bit complicated, why not something like
> `any("create table foo" in x for x in traced_statements)`?

Fine with me. I did not know that "bar" in "foobarbaz" works (I was expecting it to act as if the right hand string is a list of characters). Obviously I was wrong. Further I thought "any" was new in 2.6 and therefore not suitable for use in pysqlite which was also wrong.

> (`y in x` is simper and more readable than `x.find(y) != -1`)

I agree, I just did not know it works for substrings.

> +        sqlite3_trace(self->db, _trace_callback, trace_callback);
> +        if (PyDict_SetItem(self->function_pinboard, trace_callback, Py_None) == -1)
> +            return NULL;
>
> Shouldn't sqlite3_trace() be called only after PyDict_SetItem() succeeds?

Good catch. If SetItem fails, trace_callback may become invalid and the _trace_callback will crash. I did not think about that as I only derived that function from similar code in the module. I will have to fix this as well.
msg132552 - (view) Author: Torsten Landschoff (torsten) Date: 2011-03-29 23:02
> - you need to document the new API in Doc/library/sqlite3.rst

Included in the updated patch.

> +        self.assertTrue([x for x in traced_statements if x.find("create table foo") != -1])
>
> This looks a bit complicated, why not something like
> `any("create table foo" in x for x in traced_statements)`?

Fixed.

> +        sqlite3_trace(self->db, _trace_callback, trace_callback);
> +        if (PyDict_SetItem(self->function_pinboard, trace_callback, Py_None) == -1)
> +            return NULL;
>
> Shouldn't sqlite3_trace() be called only after PyDict_SetItem() succeeds?

Fixed as well. I just reversed the calls. What I dislike about this function pinboard approach is that every function registered as a callback stays pinned to the SQLite connection for the lifetime of the latter. But that belongs into another patch, I guess.
msg132604 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-03-30 19:01
The patch looks good to me, thank you!
Gerhard, would you like to tackle this? Otherwise I'll commit in a couple of days.
msg132608 - (view) Author: Torsten Landschoff (torsten) Date: 2011-03-30 19:40
> The patch looks good to me, thank you!
> Gerhard, would you like to tackle this? Otherwise I'll commit in a couple of days.

What I am still wondering about is if it would make sense to use the text factory here. It might make sense to pass through the bytes directly without translation into unicode (especially for big amounts).

OTOH, this is mostly a debugging aid and nothing that would be enabled in production. And premature optimization is the root of all evil...
msg132610 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-03-30 19:52
> OTOH, this is mostly a debugging aid and nothing that would be enabled
> in production. And premature optimization is the root of all evil...

Agreed.
msg132893 - (view) Author: Roundup Robot (python-dev) Date: 2011-04-03 22:13
New changeset 575ee55081dc by Antoine Pitrou in branch 'default':
Issue #11688: Add sqlite3.Connection.set_trace_callback().  Patch by Torsten Landschoff.
http://hg.python.org/cpython/rev/575ee55081dc
msg132894 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-04-03 22:14
Patch committed, thank you!
msg132903 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-04-03 23:10
Reopening, a failure appeared on some of the buildbots. I've made the failure message a bit more explicit:

test_sqlite: testing with version '2.6.0', sqlite_version '3.6.12'
[...]

======================================================================
FAIL: CheckUnicodeContent (sqlite3.test.hooks.TraceCallbackTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/buildbot/buildarea/3.x.parc-snowleopard-1/build/Lib/sqlite3/test/hooks.py", line 220, in CheckUnicodeContent
    % (ascii(unicode_value), ', '.join(map(ascii, traced_statements))))
AssertionError: False is not true : Unicode data '\xf6\xe4\xfc\xd6\xc4\xdc\xdf\u20ac' garbled in trace callback: 'create table foo(x)', 'BEGIN ', 'insert into foo(x) values (?)', 'COMMIT'


See e.g. http://www.python.org/dev/buildbot/all/builders/AMD64%20Snow%20Leopard%202%203.x/builds/168/steps/test/logs/stdio
msg132907 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-04-03 23:42
Ok, it seems that expanding the value of bound parameters in the statement passed to the trace callback is a recent SQLite feature:

http://www.sqlite.org/draft/releaselog/3_6_21.html
    “The SQL output resulting from sqlite3_trace() is now modified to include the values of bound parameters.”
msg132908 - (view) Author: Roundup Robot (python-dev) Date: 2011-04-03 23:50
New changeset ce37570768f5 by Antoine Pitrou in branch 'default':
Fix TraceCallbackTests to not use bound parameters (followup to issue #11688)
http://hg.python.org/cpython/rev/ce37570768f5
msg132910 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-04-04 00:22
Should be fixed now.
History
Date User Action Args
2011-04-04 00:22:00pitrousetstatus: open -> closed

messages: + msg132910
2011-04-03 23:50:57python-devsetmessages: + msg132908
2011-04-03 23:42:25pitrousetmessages: + msg132907
2011-04-03 23:10:30pitrousetstatus: closed -> open

messages: + msg132903
2011-04-03 22:14:12pitrousetstatus: open -> closed
messages: + msg132894

assignee: ghaering ->
resolution: fixed
stage: commit review -> resolved
2011-04-03 22:13:06python-devsetnosy: + python-dev
messages: + msg132893
2011-03-30 19:52:06pitrousetmessages: + msg132610
2011-03-30 19:40:26torstensetmessages: + msg132608
2011-03-30 19:01:00pitrousetmessages: + msg132604
stage: patch review -> commit review
2011-03-29 23:02:04torstensetfiles: + sqlite_trace.diff

messages: + msg132552
2011-03-27 17:38:22torstensetmessages: + msg132353
2011-03-27 14:31:53pitrousetversions: - Python 2.7
nosy: + pitrou

messages: + msg132332

stage: patch review
2011-03-26 22:12:45benjamin.petersonsetassignee: ghaering

nosy: + ghaering
2011-03-26 22:11:22torstensetfiles: + sqlite_trace_py27.diff

messages: + msg132276
2011-03-26 22:09:20torstencreate