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

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/