Merge lp:~stub/launchpad/pending-db-changes into lp:launchpad/db-devel

[Stuart Bishop]

SourcePackageRelease.component NOT NULL, omit OAuthNonce from replication per current production setup, Py 2.6 deprecation fix, nagios perm

Collection of minor DB tweaks.

We have stopped replicating the OAuthNonce table, and tweaks to our replication maintenance scripts to stop it being added back.

fti.py was spitting a deprecation warning under Python 2.6 - fix that.

The Bug #49717 DB patch I though had been landed had not been landed. Extend this with a tweak for the OAuthNonce table, so things still cope now it is no longer replicated.

A DB permission for our nagios checks.

[Michael Nelson (michael.nelson)]

Hi Stuart,

Just a few notes below. The main one I'm interested in is whether we should include the BuildFarmJob.status index in the db patch.

Thanks!

> === modified file 'database/replication/helpers.py'
> --- database/replication/helpers.py 2010-04-29 12:38:05 +0000
> +++ database/replication/helpers.py 2010-06-10 13:40:07 +0000
> @@ -420,7 +424,8 @@
> """ % sqlvalues(namespace, tablename))
> for namespace, tablename in cur.fetchall():
> key = (namespace, tablename)
> - if key not in tables and key not in pending_tables:
> + if (key not in tables and key not in pending_tables
> + and '%s.%s' % (namespace, tablename) not in IGNORED_TABLES):
> pending_tables.add(key)

Just out of interest, this kind of change (or these helpers) *could* be tested couldn't they?

>
> # Generate the set of sequences that are linked to any of our set of
> @@ -441,8 +446,9 @@
> ) AS whatever
> WHERE seq IS NOT NULL;
> """ % sqlvalues(fqn(namespace, tablename), namespace, tablename))
> - for row in cur.fetchall():
> - sequences.add(row[0])
> + for sequence, in cur.fetchall():

Nice, but would it be clearer to do:

           for sequence, whateverelse in cur.fetchall():

using something appropriate?

?
> + if sequence not in IGNORED_SEQUENCES:
> + sequences.add(sequence)
>
> # We can't easily convert the sequence name to (namespace, name) tuples,
> # so we might as well convert the tables to dot notation for consistancy.
>
> === modified file 'database/schema/fti.py'
> --- database/schema/fti.py 2010-05-19 18:07:56 +0000
> +++ database/schema/fti.py 2010-06-10 13:40:07 +0000
> @@ -319,18 +319,15 @@
> cmd += ' -h %s' % lp.dbhost
> if options.dbuser:
> cmd += ' -U %s' % options.dbuser
> - p = popen2.Popen4(cmd)
> - c = p.tochild
> - print >> c, "SET client_min_messages=ERROR;"
> - print >> c, "CREATE SCHEMA ts2;"
> - print >> c, open(tsearch2_sql_path).read().replace(
> - 'public;','ts2, public;'
> - )
> - p.tochild.close()
> - rv = p.wait()
> - if rv != 0:
> + p = subprocess.Popen(
> + cmd.split(' '), stdin=subprocess.PIPE,
> + stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
> + out, err = p.communicate(
> + "SET client_min_messages=ERROR; CREATE SCHEMA ts2;"
> + + open(tsearch2_sql_path).read().replace('public;','ts2, public;'))
> + if p.returncode != 0:
> log.fatal('Error executing %s:', cmd)
> - log.debug(p.fromchild.read())
> + log.debug(out)

Is err not useful here?

> sys.exit(rv)
>
> # Create ftq helper and its sibling _ftq.
>
> === added file 'database/schema/patch-2207-62-0.sql'
> --- database/schema/patch-2207-62-0.sql 1970-01-01 00:00:00 +0000
> +++ database/schema/patch-2207-62-0.sql 2010-06-10 13:40:07 +0000
> @@ -0,0 +1,14 @@
> +SET client_min_messages=ERROR;
> +
> +-- Bug #49717
> +ALTER TABLE SourcePackageRele...

review: Approve (code)

[Stuart Bishop (stub)]

On Thu, Jun 10, 2010 at 8:53 PM, Michael Nelson
<email address hidden> wrote:

>> === modified file 'database/replication/helpers.py'
>> --- database/replication/helpers.py   2010-04-29 12:38:05 +0000
>> +++ database/replication/helpers.py   2010-06-10 13:40:07 +0000
>> @@ -420,7 +424,8 @@
>>              """ % sqlvalues(namespace, tablename))
>>          for namespace, tablename in cur.fetchall():
>>              key = (namespace, tablename)
>> -            if key not in tables and key not in pending_tables:
>> +            if (key not in tables and key not in pending_tables
>> +                and '%s.%s' % (namespace, tablename) not in IGNORED_TABLES):
>>                  pending_tables.add(key)
>
> Just out of interest, this kind of change (or these helpers) *could* be tested couldn't they?

Bits could. I tend to consider staging the test environment for most
of this though, and haven't worried about the few bits that are
testable. Its all a bit ugly, but now we know it is all working I
guess we can start cleaning things up.

>>      # Generate the set of sequences that are linked to any of our set of
>> @@ -441,8 +446,9 @@
>>                  ) AS whatever
>>              WHERE seq IS NOT NULL;
>>              """ % sqlvalues(fqn(namespace, tablename), namespace, tablename))
>> -        for row in cur.fetchall():
>> -            sequences.add(row[0])
>> +        for sequence, in cur.fetchall():
>
> Nice, but would it be clearer to do:
>
>           for sequence, whateverelse in cur.fetchall():
>
> using something appropriate?

That would give an index exception, as there is only a single column
being returned.

>> +        p = subprocess.Popen(
>> +            cmd.split(' '), stdin=subprocess.PIPE,
>> +            stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
>> +        out, err = p.communicate(
>> +            "SET client_min_messages=ERROR; CREATE SCHEMA ts2;"
>> +            + open(tsearch2_sql_path).read().replace('public;','ts2, public;'))
>> +        if p.returncode != 0:
>>              log.fatal('Error executing %s:', cmd)
>> -            log.debug(p.fromchild.read())
>> +            log.debug(out)
>
> Is err not useful here?

No. It is None because I specified stderr=subprocess.STDOUT to munge
stdout and stderr together.

> I think you added a missing index to BuildFarmJob.status last Friday too. Although it may not have helped the direct issue there, I think it should still be included. Do you?

That has already landed as patch-2207-60-1.sql. Its in a different db
patch as it has already been applied to production.

--
Stuart Bishop <email address hidden>
http://www.stuartbishop.net/

=== modified file 'database/replication/helpers.py'
--- database/replication/helpers.py	2010-04-29 12:38:05 +0000
+++ database/replication/helpers.py	2010-06-10 12:51:38 +0000
@@ -44,7 +44,6 @@
     ('public', 'nameblacklist'),
     ('public', 'openidconsumerassociation'),
     ('public', 'openidconsumernonce'),
-    ('public', 'oauthnonce'),
     ('public', 'codeimportmachine'),
     ('public', 'scriptactivity'),
     ('public', 'standardshipitrequest'),
@@ -71,6 +70,8 @@
     # Database statistics
     'public.databasetablestats',
     'public.databasecpustats',
+    # Don't replicate OAuthNonce - too busy and no real gain.
+    'public.oauthnonce',
     # Ubuntu SSO database. These tables where created manually by ISD
     # and the Launchpad scripts should not mess with them. Eventually
     # these tables will be in a totally separate database.
@@ -353,6 +354,9 @@
 
     A replication set must contain all tables linked by foreign key
     reference to the given table, and sequences used to generate keys.
+    Tables and sequences can be added to the IGNORED_TABLES and
+    IGNORED_SEQUENCES lists for cases where we known can safely ignore
+    this restriction.
 
     :param seeds: [(namespace, tablename), ...]
 
@@ -420,7 +424,8 @@
             """ % sqlvalues(namespace, tablename))
         for namespace, tablename in cur.fetchall():
             key = (namespace, tablename)
-            if key not in tables and key not in pending_tables:
+            if (key not in tables and key not in pending_tables
+                and '%s.%s' % (namespace, tablename) not in IGNORED_TABLES):
                 pending_tables.add(key)
 
     # Generate the set of sequences that are linked to any of our set of
@@ -441,8 +446,9 @@
                 ) AS whatever
             WHERE seq IS NOT NULL;
             """ % sqlvalues(fqn(namespace, tablename), namespace, tablename))
-        for row in cur.fetchall():
-            sequences.add(row[0])
+        for sequence, in cur.fetchall():
+            if sequence not in IGNORED_SEQUENCES:
+                sequences.add(sequence)
 
     # We can't easily convert the sequence name to (namespace, name) tuples,
     # so we might as well convert the tables to dot notation for consistancy.

=== modified file 'database/schema/fti.py'
--- database/schema/fti.py	2010-05-19 18:07:56 +0000
+++ database/schema/fti.py	2010-06-10 12:51:38 +0000
@@ -14,10 +14,10 @@
 import _pythonpath
 
 from distutils.version import LooseVersion
-import sys
 import os.path
 from optparse import OptionParser
-import popen2
+import subprocess
+import sys
 from tempfile import NamedTemporaryFile
 from textwrap import dedent
 import time
@@ -319,18 +319,15 @@
             cmd += ' -h %s' % lp.dbhost
         if options.dbuser:
             cmd += ' -U %s' % options.dbuser
-        p = popen2.Popen4(cmd)
-        c = p.tochild
-        print >> c, "SET client_min_messages=ERROR;"
-        print >> c, "CREATE SCHEMA ts2;"
-        print >> c, open(tsearch2_sql_path).read().replace(
-                'public;','ts2, public;'
-                )
-        p.tochild.close()
-        rv = p.wait()
-        if rv != 0:
+        p = subprocess.Popen(
+            cmd.split(' '), stdin=subprocess.PIPE,
+            stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
+        out, err = p.communicate(
+            "SET client_min_messages=ERROR; CREATE SCHEMA ts2;"
+            + open(tsearch2_sql_path).read().replace('public;','ts2, public;'))
+        if p.returncode != 0:
             log.fatal('Error executing %s:', cmd)
-            log.debug(p.fromchild.read())
+            log.debug(out)
             sys.exit(rv)
 
     # Create ftq helper and its sibling _ftq.

=== added file 'database/schema/patch-2207-62-0.sql'
--- database/schema/patch-2207-62-0.sql	1970-01-01 00:00:00 +0000
+++ database/schema/patch-2207-62-0.sql	2010-06-10 12:51:38 +0000
@@ -0,0 +1,14 @@
+SET client_min_messages=ERROR;
+
+-- Bug #49717
+ALTER TABLE SourcePackageRelease ALTER component SET NOT NULL;
+
+-- We are taking OAuthNonce out of replication, so we make the foreign
+-- key reference ON DELETE CASCADE so things don't explode when we
+-- shuffle the lpmain master around.
+ALTER TABLE OAuthNonce DROP CONSTRAINT oauthnonce__access_token__fk;
+ALTER TABLE OAuthNonce ADD CONSTRAINT oauthnonce__access_token__fk
+    FOREIGN KEY (access_token) REFERENCES OAuthAccessToken
+    ON DELETE CASCADE;
+
+INSERT INTO LaunchpadDatabaseRevision VALUES (2207, 62, 0);

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2010-06-07 10:24:03 +0000
+++ database/schema/security.cfg	2010-06-10 12:51:38 +0000
@@ -1826,6 +1826,7 @@
 type=user
 public.archive                          = SELECT
 public.buildfarmjob                     = SELECT
+public.databasereplicationlag           = SELECT
 public.packagebuild                     = SELECT
 public.binarypackagebuild               = SELECT
 public.buildqueue                       = SELECT