Check for table existence in DatabaseManager.upgrade_tables

[Ryan J Ollos]

If upgrade_tables checked for table existence before attempting to copy the data to a temporary table, then upgrade_tables could handle a mix of new and revised tables. This would make the upgrade function slightly similar for a case such as that demonstrated in db4.py for SpamFilter​.

[Ryan J Ollos]

Committed to trunk in r15316.

[Jun Omae]

I think the table4 should be removed in ModifyTableTestCase.tearDown().

In addition, table names in MySQL on Windows are case-insensitive. The changes in [15316] may not work as expected.

.get_column_names() with non-existent table returns an empty list. We could check whether the returned value is empty.

trac/db/api.py

@@ -357 +357 @@
 
         :since: version 1.2
         """
-        existing_table_names = self.get_table_names()
         for new_table in new_schema:
             temp_table_name = new_table.name + '_old'
             old_column_names = set(self.get_column_names(new_table))
             new_column_names = {col.name for col in new_table.columns}
             cols_to_copy = ','.join(old_column_names & new_column_names)
             with self.env.db_transaction as db:
-                has_table = new_table.name in existing_table_names
+                has_table = bool(old_column_names)
                 if has_table:
                     cursor = db.cursor()
                     cursor.execute("""

I think .get_column_names() may be better to raise an exception if the given table is not existent….

[Ryan J Ollos]

Replying to Jun Omae:

I think the table4 should be removed in ModifyTableTestCase.tearDown().

Make sense. I'm sure I had added this, so I'm stumped as to why it wasn't in my final changes.

In addition, table names in MySQL on Windows are case-insensitive. The changes in [15316] may not work as expected.

Perhaps we should add has_table(...) method to DatabaseManager and ConnectionBase?

I think .get_column_names() may be better to raise an exception if the given table is not existent….

I agree with .get_column_names() raising exception when table doesn't exist.

[Jun Omae]

Replying to Ryan J Ollos:

Replying to Jun Omae:

I think the table4 should be removed in ModifyTableTestCase.tearDown().

Make sense. I'm sure I had added this, so I'm stumped as to why it wasn't in my final changes.

See [b051fb622/jomae.git].

In addition, table names in MySQL on Windows are case-insensitive. The changes in [15316] may not work as expected.

Perhaps we should add has_table(...) method to DatabaseManager and ConnectionBase?

Sounds good. See [ead3a3383/jomae.git].

I think .get_column_names() may be better to raise an exception if the given table is not existent….

I agree with .get_column_names() raising exception when table doesn't exist.

Thanks. I consider .drop_columns() also should raise an exception with missing table, [f03bc446c/jomae.git].

Proposed changes in jomae.git@t12643.1.

[Ryan J Ollos]

Looks good, thanks for fixing. I might suggest changing the error message to Table "blah" not found, consistent with tags/trac-1.2/trac/db/sqlite_backend.py@:302#L298​.

[Jun Omae]

Thanks for the suggesting. Committed changes with your suggestions in [15322].

[Ryan J Ollos]

I saw a failure executing db44.py on teo (Postgres 9.4). I grabbed the database and reproduced locally.

[pid 38343 140737226027968] 19:30:27 Trac[console] ERROR: Exception in trac-admin command: u'upgrade'
Traceback (most recent call last):
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/admin/console.py", line 112, in onecmd
    rv = cmd.Cmd.onecmd(self, line) or 0
  File "/usr/local/Cellar/python/2.7.13/Frameworks/Python.framework/Versions/2.7/lib/python2.7/cmd.py", line 220, in onecmd
    return self.default(line)
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/admin/console.py", line 291, in default
    return self.cmd_mgr.execute_command(*args)
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/admin/api.py", line 135, in execute_command
    return f(*fargs)
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/env.py", line 999, in _do_upgrade
    self.env.upgrade(backup=no_backup is None)
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/env.py", line 738, in upgrade
    participant.upgrade_environment()
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/env.py", line 779, in upgrade_environment
    pkg='trac.upgrades')
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/db/api.py", line 532, in upgrade
    script.do_upgrade(self.env, i, cursor)
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/upgrades/db44.py", line 38, in do_upgrade
    DatabaseManager(env).upgrade_tables(new_schema)
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/db/api.py", line 392, in upgrade_tables
    self.create_tables((new_table,))
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/db/api.py", line 302, in create_tables
    db(sql)
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/db/util.py", line 129, in execute
    cursor.execute(query, params if params is not None else [])
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/db/util.py", line 74, in execute
    return self.cursor.execute(sql)
ProgrammingError: relation "enum" already exists
ProgrammingError: relation "enum" already exists

has_tables returns False and self.schema is None: trunk/trac/db/postgres_backend.py@15420:389#L385​. It seems that self.schema should be public:

> SELECT * FROM information_schema.columns WHERE table_name='enum';
+-----------------+----------------+--------------+---------------+-------------
| table_catalog   | table_schema   | table_name   | column_name   |   ordinal_po
|-----------------+----------------+--------------+---------------+-------------
| teo             | public         | enum         | type          |
| teo             | public         | enum         | name          |
| teo             | public         | enum         | value         |
+-----------------+----------------+--------------+---------------+-------------
SELECT 3

The upgrade succeeds if I append ?schema=public to the database string. However, I think that teo has not used ?schema=public in the connection string and it has worked previous. TracEnvironment says:

Trac uses the public schema by default…

[Ryan J Ollos]

See #12838 for more discussion about comment:11.