Edgewall Software
Modify

Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#12643 closed defect (fixed)

Check for table existence in DatabaseManager.upgrade_tables

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.3.2
Component: database backend Version:
Severity: normal Keywords:
Cc: Branch:
Release Notes:
API Changes:
  • DatabaseManager.upgrade_tables checks table existence before migrating data for a table, allowing new tables to be specified in schema.
  • Added has_table method to DatabaseManager and ConnectionBase.
  • get_column_names and drop_columns of DatabaseManager raises an OperationalError when the given table is missing.

Description

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.

Attachments (0)

Change History (12)

comment:1 by Ryan J Ollos, 3 years ago

Summary: Check for table existence in DatabaseManager.ugrade_tablesCheck for table existence in DatabaseManager.upgrade_tables

comment:2 by Ryan J Ollos, 3 years ago

Milestone: next-stable-1.2.x1.2.1
Owner: set to Ryan J Ollos
Status: newassigned

comment:3 by Ryan J Ollos, 3 years ago

API Changes: modified (diff)

comment:4 by Ryan J Ollos, 3 years ago

Milestone: 1.2.11.3.2

comment:5 by Ryan J Ollos, 3 years ago

Resolution: fixed
Status: assignedclosed

Committed to trunk in r15316.

comment:6 by Jun Omae, 3 years ago

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

    diff --git a/trac/db/api.py b/trac/db/api.py
    index 39dead193..68fd485c1 100644
    a b class DatabaseManager(Component):  
    357357
    358358        :since: version 1.2
    359359        """
    360         existing_table_names = self.get_table_names()
    361360        for new_table in new_schema:
    362361            temp_table_name = new_table.name + '_old'
    363362            old_column_names = set(self.get_column_names(new_table))
    364363            new_column_names = {col.name for col in new_table.columns}
    365364            cols_to_copy = ','.join(old_column_names & new_column_names)
    366365            with self.env.db_transaction as db:
    367                 has_table = new_table.name in existing_table_names
     366                has_table = bool(old_column_names)
    368367                if has_table:
    369368                    cursor = db.cursor()
    370369                    cursor.execute("""

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

in reply to:  6 ; comment:7 by Ryan J Ollos, 3 years ago

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.

in reply to:  7 comment:8 by Jun Omae, 3 years ago

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.

comment:9 by Ryan J Ollos, 3 years ago

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.

comment:10 by Jun Omae, 3 years ago

API Changes: modified (diff)

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

comment:11 by Ryan J Ollos, 2 years ago

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…

Last edited 2 years ago by Ryan J Ollos (previous) (diff)

comment:12 by Ryan J Ollos, 2 years ago

See #12838 for more discussion about comment:11.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.
to as closed The owner will be changed from Ryan J Ollos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.