Code review comment for lp:~stub/launchpad/pending-db-changes

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 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);

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?

https://irclogs.canonical.com/2010/06/04/%23launchpad-code.html#t10:37