Misc. fixes to migration revision scripts

[jdavcs]

These fixes were triggered by a variety of migration errors experienced independently by @ahmedhamidawan and myself in (at least) two different contexts while moving between revisions. These fixes should address all or most of the root causes.

Inlcuded:

• Consistent use of checking if object exists (when adding) and not exists (when dropping). Use case: when the alembic version gets out of sync with the state of the database (frequently happens during development), it was impossible to just ./manage_db.sh upgrade (or downgrade) because most conflicting steps raised an error. This fixes it: migration steps that conflict with the state of the database will be noops, with appropriate messages logged. Also, now one can simply delete the rows from alembic_version and run ./manage_db.sh upgrade: all migrations will be re-applied regardless of the state of the database.
• Consistent use of batch mode for alter operations: this is required to make column additions/drops possible under SQLite; this was used for adding columns, but not dropping (this may have caused inconsistent sqlite database state in dev environments).
• Disable offline mode for alter operations: this raises an error under sqlite: there's no elegant fix for this.
• Reasonable refactoring and prettifying of affected scripts and the util module.

It seems reasonable to target release_23.0, as this fixes some bugs; but I can re-target to dev if needed.

How to test the changes?

(Select all options that apply)

• I've included appropriate automated tests.
• This is a refactoring of components with existing test coverage.
• Instructions for manual testing are as follows:
  1. Assuming your database is postgresql. On dev: psql -c "\c your-database" -c "delete from alembic_version;" This empties the version table
  2. ./manage_db.sh upgrade Observe error.
  3. Switch to this branch, run the upgrade script again: there will be no error. Check the database version to verify (./manage_db.sh dbversion or psql to your database)
  4. Change your database_connection in config/galaxy.yml to point to a sqlite database. Then redo the steps above (using sqlite3 instead of psql).

License

• I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

[jdavcs]

package test failure unrelated.

[mvdbeek]

I don't know if this is the right approach, it seems like you'd easily be left with columns that were never merged into a stable release, or columns that don't have the right type. If you're working on migrations yourself you check out the branch that added the migration and you downgrade, it has worked like that since we've added migrations, I am worried about hard to predict database schemas here.

[jdavcs]

I don't know if this is the right approach, it seems like you'd easily be left with columns that were never merged into a stable release, or columns that don't have the right type. If you're working on migrations yourself you check out the branch that added the migration and you downgrade, it has worked like that since we've added migrations, I am worried about hard to predict database schemas here.

We do it, just inconsistently (here, here and here: we check when upgrading, but we don't check when downgrading - which was an oversight). In the past (pre-alembic), we did it for everything (like this): we logged an exception, but proceeded without raising one.

I see your point. But I think the logic here is harmless. Here's what happens:

• add NEW object: if that object already exists in the database, log a message and skip the step
• drop EXISTING object: if that object does not exist in the database, log a message and skip the step

It is, actually, much tighter than previously (where we logged any exception and proceeded): if a step fails, migrations don't proceed.

It does not apply to altering a column. When we add that type of revision, we won't be checking for column existence.

If I end up with an invalid alembic_version table entry (for whatever reason), I want to be able to manage_db.sh upgrade and have all relevant/missing revisions re-applied. Of course, there is the possibility of a revision adding some table Foo, with Foo already existing in the database; but the likelihood of that is probably the same as that of any table being manually edited - which is something we can't catch anyway (we don't reflect the database schema).

In summary, I think this approach is reasonably safe (and consistent), but I'm absolutely open to considering other approaches. (As a minor change, do you think log.exception is more appropriate than log.info?)

[mvdbeek]

• This fixes it: migration steps that conflict with the state of the database will be noops, with appropriate messages logge

That doesn't seem safe to me. The things you've linked seem suboptimal, but we're probably copied from someplace ?

• add NEW object: if that object already exists in the database, log a message and skip the step

How is this safe ? If you look at the else branch of drop_index and add_column, do you really think that is safe ?

If I end up with an invalid alembic_version table entry (for whatever reason),

That's the big thing I think we're doing wrong, how can this happen outside of an alembic bug ?

I want to be able to manage_db.sh upgrade and have all relevant/missing revisions re-applied

How can that possibly be safe if the code doesn't know a revision ?

[jdavcs]

Closed in favor of #15811