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

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...

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/