launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/profile-sql-bind-values into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/profile-sql-bind-values/+merge/47955

A small additional extension to the previous work to optionally log sql bind various when doing sql tracing. This branch ensures that sql statements stuffed into the oops via the browser request timeline also have the bind variables substituted. It was done to make ++profile++ reports more useful.

There was a question raised as to whether oops-tools would break in terms of its sql processing 
eg it accumulates time taken to execute any given sql statement (ignoring bind parameters). The oops-tools SME indicated things would still work there, plus I did a code read and the sql processing logic includes a function to convert statements from things like "select foo from bar where thing = 2" to "select foo from bar where thing = $INT" etc. So oops-tools can handle the change.

Now comes a question - is there even any need to make the logging of sql with bind variables an option anymore? An environment variable LP_DEBUG_SQL_VALUES was introduced to allow this to be turned on (off by default) but I think it should just be always on - it appears the tools work either way and there's much better value providing the bind variables when analysing issues, be they performance or logic issues. If it's agreed, I'll remove the option and make it always on before landing this branch.


-- 
https://code.launchpad.net/~wallyworld/launchpad/profile-sql-bind-values/+merge/47955
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/profile-sql-bind-values into lp:launchpad.

=== modified file 'lib/canonical/launchpad/webapp/adapter.py'
--- lib/canonical/launchpad/webapp/adapter.py	2010-12-03 13:12:34 +0000
+++ lib/canonical/launchpad/webapp/adapter.py	2011-01-31 01:26:45 +0000
@@ -24,7 +24,10 @@
     QueryCanceledError,
     )
 import pytz
-from storm.database import register_scheme
+from storm.database import (
+    Connection,
+    register_scheme,
+    )
 from storm.databases.postgres import (
     Postgres,
     PostgresConnection,
@@ -607,12 +610,12 @@
         if self._debug_sql_extra:
             traceback.print_stack()
             sys.stderr.write("." * 70 + "\n")
+        param_strings = Connection.to_database(params)
+        statement_with_values = statement % tuple(param_strings)
         if self._debug_sql or self._debug_sql_extra:
             stmt_to_log = statement
             if self._debug_sql_bind_values:
-                from storm.database import Connection
-                param_strings = Connection.to_database(params)
-                stmt_to_log = statement % tuple(param_strings)
+                stmt_to_log = statement_with_values
             sys.stderr.write(stmt_to_log + "\n")
             sys.stderr.write("-" * 70 + "\n")
         # store the last executed statement as an attribute on the current
@@ -622,7 +625,7 @@
         if request_starttime is None:
             return
         action = get_request_timeline(get_current_browser_request()).start(
-            'SQL-%s' % connection._database.name, statement)
+            'SQL-%s' % connection._database.name, statement_with_values)
         connection._lp_statement_action = action
 
     def connection_raw_execute_success(self, connection, raw_cursor,