launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/private-team-timeout-1068533 into lp:launchpad.

Commit message:
Reduce the number of queries required to do visibilityConsistencyWarning check for a team.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1068533 in Launchpad itself: "Timeout creating private team or making a public team private"
  https://bugs.launchpad.net/launchpad/+bug/1068533

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/private-team-timeout-1068533/+merge/131129

== Implementation ==

postgresql.listReferences() is called to see what FKs may reference a team. The listReferences() actually finds all indirect references as well. Not only are these indirect references not needed here, but an extra 188 or so queries is executed to find them. So improvement #1, add an option to not bother finding indirect references. This reduces this bit to own query.

The second issue is that for each of the 189 or so possible references, a separate query is done to see if the person is referenced. I converted this to a single union query instead.

So net effect, > 360 queries reduced to 2.

== Tests ==

Internal change, existing tests should do the trick.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/model/person.py
  lib/lp/services/database/postgresql.py

-- 
https://code.launchpad.net/~wallyworld/launchpad/private-team-timeout-1068533/+merge/131129
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/private-team-timeout-1068533 into lp:launchpad.

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2012-10-22 02:30:44 +0000
+++ lib/lp/registry/model/person.py	2012-10-24 03:56:20 +0000
@@ -2219,7 +2219,8 @@
             return self._visibility_warning_cache
 
         cur = cursor()
-        references = list(postgresql.listReferences(cur, 'person', 'id'))
+        references = list(
+            postgresql.listReferences(cur, 'person', 'id', indirect=False))
         # These tables will be skipped since they do not risk leaking
         # team membership information, except StructuralSubscription
         # which will be checked further down to provide a clearer warning.
@@ -2262,17 +2263,24 @@
                          ])
 
         warnings = set()
+        ref_query = []
         for src_tab, src_col, ref_tab, ref_col, updact, delact in references:
             if (src_tab, src_col) in skip:
                 continue
-            cur.execute('SELECT 1 FROM %s WHERE %s=%d LIMIT 1'
-                        % (src_tab, src_col, self.id))
-            if cur.rowcount > 0:
-                if src_tab[0] in 'aeiou':
+            ref_query.append(
+                "SELECT '%(table)s' AS table FROM %(table)s "
+                "WHERE %(col)s = %(person_id)d"
+                % {'col': src_col, 'table': src_tab, 'person_id': self.id})
+        if ref_query:
+            cur.execute(' UNION '.join(ref_query))
+            for src_tab in cur.fetchall():
+                table_name = (
+                    src_tab[0] if isinstance(src_tab, tuple) else src_tab)
+                if table_name[0] in 'aeiou':
                     article = 'an'
                 else:
                     article = 'a'
-                warnings.add('%s %s' % (article, src_tab))
+                warnings.add('%s %s' % (article, table_name))
 
         # Private teams may have structural subscription, so the following
         # test is not applied to them.

=== modified file 'lib/lp/services/database/postgresql.py'
--- lib/lp/services/database/postgresql.py	2012-01-06 11:08:30 +0000
+++ lib/lp/services/database/postgresql.py	2012-10-24 03:56:20 +0000
@@ -17,7 +17,7 @@
     )
 
 
-def listReferences(cur, table, column, _state=None):
+def listReferences(cur, table, column, indirect=True, _state=None):
     """Return a list of all foreign key references to the given table column
 
     `table` and `column` are both case sensitive strings (so they should
@@ -27,9 +27,10 @@
 
     returns `[(from_table, from_column, to_table, to_column, update, delete)]`
 
-    `from` entries refer to the `to` entries. This method is recursive -
-    not only does it return all references to the given table column, but
-    also all references to those references etc. (indirect references).
+    `from` entries refer to the `to` entries. If indirect is True, the this
+    method is recursive - not only does it return all references to the given
+    table column, but also all references to those references etc.
+    ie (indirect references).
 
     `update` is the update clause (eg. on update cascade)
     `delete` is the delete clause (eg. on delete cascade)
@@ -92,8 +93,9 @@
         # Avoid loops:
         if t not in _state:
             _state.append(t)
-            # Recurse, Locating references to the reference we just found.
-            listReferences(cur, t[0], t[1], _state)
+            if indirect:
+                # Recurse, Locating references to the reference we just found.
+                listReferences(cur, t[0], t[1], indirect, _state)
     # Don't sort. This way, we return the columns in order of distance
     # from the original (table, column), making it easier to change keys
     return _state