Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge conflicting db migration branches into one #15771

Merged

Conversation

jdavcs
Copy link
Member

@jdavcs jdavcs commented Mar 12, 2023

This is meant to replace #15768.

The core issue is explained in #15768. Here's a visualization of what happened and the 2 solutions. From left to right:

  1. 23.0: 2 revisions added on top of c39f
  2. dev_pre_conflict: 2 other revisions added on top of c39f
  3. dev_conflict: 23.0 merged into dev: we have 2 heads: caa7 and 9540.
    Why is this a problem? Because when we want to upgrade, alembic can't know what head we want to upgrade to.
  4. dev_solved_renamed: initially proposed solution: manually edit the down_revision value in d058, "rebasing" it onto caa7, thus rearranging the graph to have 1 gxy head
  5. dev_solved_merged: alternative solution: add an empty merge revision
    (with ./scripts/run_alembic.sh merge caa7 9540), thus restoring 1 gxy head

image

Why renaming is not a good solution
If we start with the pre-conflict dev branch (normal case for a development setup), the alembic_version table in the database will contain revisions 9540 and d4a6 (d4a6 is the tsi branch, so it's irrelevant in this context). Once we pull from upstream, we get the 2 new revisions from the merged-in 23.0 branch (3a29 and caa7), resulting in 2 gxy heads. We rename the down_migration in d058, effectively hiding 3a29 and caa7 from the database (@natefoo that's the problem case you referred to). So, running `manage_db.sh upgrade will be a noop, and the 2 indexes won't be added.

By adding an explicit merge revision, we reconcile the diverged branches, while ensuring that revisions won't be silenced.

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:
    No easy way to set this up. Here's gist with a transcript of some of the testing I did of this approach. In summary:
  • setup state:
    • branch: pre-conflict dev (current dev minus 3a29, caa7
    • brand new database (I ran this on sqlite and postgresql) via manage_db.sh init
  • move to conflict state (add 2 migrations from 23.0)
  • try to upgrade: verify error
  • add a merge revision
  • upgrade: no error
  • downgrade to 23.0
  • upgrade back to dev
    (verify version, dbversion, and history after each step).

License

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

@jdavcs jdavcs added kind/bug area/database Galaxy's database or data access layer labels Mar 12, 2023
@jdavcs jdavcs added this to the 23.1 milestone Mar 12, 2023
@martenson
Copy link
Member

martenson commented Mar 12, 2023

Given these are "revisions" merging makes a lot of sense. After merging this PR will alembic know how to get from 9540 to caa7 (last graph, second from the bottom)?

@jdavcs
Copy link
Member Author

jdavcs commented Mar 12, 2023

Given these are "revisions" merging makes a lot of sense. After merging this PR will alembic know how to get from 9540 to caa7 (last graph, second from the bottom)?

Like this: ./manage_db.sh downgrade caa7 if you are at the merged head. If you are on 9540, then just upgrade. Here's what I see:

(.venv) rivendell$ ./manage_db.sh dbversion
Activating virtualenv at .venv
INFO:alembic.runtime.migration:Context impl SQLiteImpl.
INFO:alembic.runtime.migration:Will assume non-transactional DDL.
9540a051226e
d4a650f47a3c (head)
(.venv) rivendell$ ./manage_db.sh upgrade caa7
Activating virtualenv at .venv
INFO:alembic.runtime.migration:Context impl SQLiteImpl.
INFO:alembic.runtime.migration:Will assume non-transactional DDL.
INFO:alembic.runtime.migration:Running upgrade c39f1de47a04 -> 3a2914d703ca, add index workflow_request_step_states__workflow_invocation_id
DEBUG:alembic.runtime.migration:new branch insert 3a2914d703ca
INFO:alembic.runtime.migration:Running upgrade 3a2914d703ca -> caa7742f7bca, add index workflow_request_input_parameters__workflow_invocation_id
DEBUG:alembic.runtime.migration:update 3a2914d703ca to caa7742f7bca
(.venv) rivendell$ ./manage_db.sh dbversion
Activating virtualenv at .venv
INFO:alembic.runtime.migration:Context impl SQLiteImpl.
INFO:alembic.runtime.migration:Will assume non-transactional DDL.
caa7742f7bca
9540a051226e
d4a650f47a3c (head)

There will be errors on some steps (as per #15772), but those are not related to this merging.

Copy link
Contributor

@guerler guerler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolves the issue for me, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/database Galaxy's database or data access layer kind/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants