Downgrading with multiple Dependencies on a revision throws exception when these dependencies are referenced from other branches

[sqlalchemy-bot]

Migrated issue, originally created by cloew (@cloew)

When implementing a migration schema via Branch Dependencies, you can get into a position where downgrading is impossible.

The culprit seems to be that any migration with more than 1 dependency is treated as a merge, but it is possible that all the dependencies are actually required by other heads. This causes the unmerge to break because it cannot find a revision to move the head for the current revision's branch to.

Attached is a simple set of Alembic migrations that result in the mentioned error

---

Attachments: patch2.txt | alembic-test.zip | patch.txt

[sqlalchemy-bot]

Michael Bayer (@zzzeek) wrote:

nice test! here's a graph:

#!

<base> -> d6569cacc59f --+--> 636e696161d9, b1-1 --> 68f85e842497, b1-2
                         |         ^                       |
                         |         |   +-------------------+
                         |         |   |
                         |         +---|-------------------+
                         |             |                   |
                         |             v                   |
                         +--> 15acdfa5f544, b2-1 --> 642cb272cc66, b2-2 --> 2f03d00150b5, b2-3

my first impression was that there was some mathematical impossibility going on, but looking at the graph it seems like there's just some treatment of a "dependency" more like a downgrade target that isn't reachable.

[sqlalchemy-bot]

Changes by Michael Bayer (@zzzeek):

• set milestone to "fasttrack"

[sqlalchemy-bot]

cloew (@cloew) wrote:

Yeah, form what I could tell. The problem is that both of 642c's dependencies are included in the b1 branch.
So, when it tries to downgrade it sees that it has multiple child revisions, but it can't actually move the head to either of them because they are part of the b1 branches head.
So, when downgrading from 642c it tries to move the head to either 15ac or 636e, but can't because their already included in the b1 head.
It looks like it actually should be deleting the head for b2 at that point.

In Alembic 0.8.6, I saw that the RevisionStep.should_delete_branch always treats merges (things with multiple dependencies) as points where new heads should be created.
It never tries to delete a head from there. I was able to hack it to get it to work by checking to see if the merge revision's dependencies were all included in other branches.
But, it looked like some of that logic already happens when there's a single revision, so I didn't know how the two should interact.

[sqlalchemy-bot]

Michael Bayer (@zzzeek) wrote:

patch to play with

[sqlalchemy-bot]

Changes by Michael Bayer (@zzzeek):

• attached file patch.txt

[sqlalchemy-bot]

Michael Bayer (@zzzeek) wrote:

another patch to play with

[sqlalchemy-bot]

Changes by Michael Bayer (@zzzeek):

• attached file patch2.txt

[sqlalchemy-bot]

Michael Bayer (@zzzeek) wrote:

i can get this series to work but not without doing things that break some of the traversal tests. will have to find some more time to look.

[sqlalchemy-bot]

Michael Bayer (@zzzeek) wrote:

proposed change is at https://gerrit.sqlalchemy.org/#/c/146/. This is not a quick one and really changes things dramatically, though hopefully I can get it all to be limited to just downgrades which is not as critical of a use case. I'm hoping you've worked around your issue downstream in some way as this might take a few weeks for me to get enough time.

[sqlalchemy-bot]

cloew (@cloew) wrote:

Thanks for the quick response! And ya, I think I've got a couple of things to try to make it not an issue for the time being.
It definitely sounds like there's a good deal of work to get it working right.

I was a little surprised at the solution. I was thinking the problem was the 642c version not being flagged for deletion.
Instead the code was trying to update the alembic_version to one of its children, but both children (15ac, 636e) are part of the b1 head, so it explodes.

When I was tinkering with this yesterday, I got a patch that fixed the problem for me just by fixing the above and it seemed to put the alembic_version table in the proper state.
I only needed to modify the ReveisionStep.should_delete_branch method.
Here's the new function in case it helps.

    def should_delete_branch(self, heads):
        if not self.is_downgrade:
            return False

        if self.revision.revision not in heads:
            return False

        downrevs = self.revision._all_down_revisions
        print(downrevs)
        if not downrevs:
            # is a base
            return True
        elif len(downrevs) == 1:
            downrev = self.revision_map.get_revision(downrevs[0])

            if not downrev._is_real_branch_point:
                return False

            descendants = set(
                r.revision for r in self.revision_map._get_descendant_nodes(
                    self.revision_map.get_revisions(downrev._all_nextrev),
                    check=False
                )
            )

            # the downrev is a branchpoint, and other members or descendants
            # of the branch are still in heads; so delete this branch.
            # the reason this occurs is because traversal tries to stay
            # fully on one branch down to the branchpoint before starting
            # the other; so if we have a->b->(c1->d1->e1, c2->d2->e2),
            # on a downgrade from the top we may go e1, d1, c1, now heads
            # are at c1 and e2, with the current method, we don't know that
            # "e2" is important unless we get all descendants of c1/c2

            if len(descendants.intersection(heads).difference(
                    [self.revision.revision])):
            # TODO: this doesn't work; make sure tests are here to ensure
            # this fails
            #if len(downrev._all_nextrev.intersection(heads).difference(
            #        [self.revision.revision])):

                return True
            else:
                return False
        else: # Below here is what I changed CLOEW
            other_heads = set(heads).difference([self.revision.revision])
            if not other_heads:
                # is a true merge point
                return False
            else:
                # Dependencies are potentially children of other heads
                # If all of them are included in other heads, then we need to delete this branch
                ancestors = set(
                    r.revision for r in
                    self.revision_map._get_ancestor_nodes(
                        self.revision_map.get_revisions(other_heads),
                        check=False
                    )
                )
                to_revisions = list(set(self.to_revisions).difference(ancestors))
                if to_revisions:
                    return False
                else:
                    return True

Sorry if I'm misunderstanding things and thanks for your time and awesome libraries!

[sqlalchemy-bot]

Michael Bayer (@zzzeek) wrote:

I think you're understanding just fine, Im the one who can't keep this system in my head, I just went through trying a whole new branch yesterday which possibly gets closer to fixing this but that one also has problems. I've decided against changing how rows are related to versions in the alembic_versions table, for example - anytime a particular number is in there, everything that's dependent on it is implied and we should keep that consistent.

Certainly the "unmerge branch" thing is part of what needs to be opened up here and my more recent patch has that too. Hopefully I'll be able to put up something new today.

[sqlalchemy-bot]

Michael Bayer (@zzzeek) wrote:

OK, now I understand the problem, but due to all the work I did there is the actual resolution to this, then there's a new feature that is actually separate.

So the actual bug is that, we're on 68f and 642 as heads. We tell the system, let's go to 15ac. The revision traversal thing works with this, and in current release calculates that the revisions we want to deal with are just those three, 68f, 642, 15ac. It does not include 636 (so technically, we only need to deal with 642 and 15ac, and not 68f at all, but that's not where the actual failure comes from here). The alembic_version updater (migration.py) sees that for us to go from 642 to 15ac, that's in its (incorrect) view a "merge point", e.g. where multiple revisions come together as one - in this case it sees 642 as a merge between 636 and 15ac. When really, it's not, 636 is not a version, it's a version with a "dependency" on 642 - which is a concept that was added on top of the original branching logic. The hackish addition of "dependency" as a modified notion of a revision with multiple bases is where this kind of mismatch starts.

So what has to happen is that where we check that "hey this is a mergepoint to be unmerged", we look at the list of revs we're dealing with and see that hey, half of our mergepoint isn't here, so we really can't unmerge, we just need to delete. A patch against master (current 0.8.6 or as of master ccf3ca5) that fixes this is:

diff --git a/alembic/runtime/migration.py b/alembic/runtime/migration.py
index e811a36..f3b00c9 100644
--- a/alembic/runtime/migration.py
+++ b/alembic/runtime/migration.py
@@ -618,9 +618,31 @@ class RevisionStep(MigrationStep):
                 return True
             else:
                 return False
+        elif len(self.revision._versioned_down_revisions) > 1:
+             # is a merge point
+             return False
         else:
-            # is a merge point
-            return False
+            # is a version that others are dependent on.
+            # calculate effective "to revisions", and if there
+            # are some, then we'd need to do an "unmerge", not a
+            # delete.
+            to_revisions = self._unmerge_to_revisions(heads)
+            return not to_revisions
+
+    def _unmerge_to_revisions(self, heads):
+        other_heads = set(heads).difference([self.revision.revision])
+        if other_heads:
+            ancestors = set(
+                r.revision for r in
+                self.revision_map._get_ancestor_nodes(
+                    self.revision_map.get_revisions(other_heads),
+                    check=False
+                )
+            )
+
+            return list(set(self.to_revisions).difference(ancestors))
+        else:
+            return self.to_revisions
 
     def merge_branch_idents(self, heads):
         other_heads = set(heads).difference(self.from_revisions)
@@ -645,19 +667,7 @@ class RevisionStep(MigrationStep):
         )
 
     def unmerge_branch_idents(self, heads):
-        other_heads = set(heads).difference([self.revision.revision])
-        if other_heads:
-            ancestors = set(
-                r.revision for r in
-                self.revision_map._get_ancestor_nodes(
-                    self.revision_map.get_revisions(other_heads),
-                    check=False
-                )
-            )
-            to_revisions = list(set(self.to_revisions).difference(ancestors))
-        else:
-            to_revisions = self.to_revisions
-
+        to_revisions = self._unmerge_to_revisions(heads)
         return (
             # update from rev, update to rev, insert revs
             self.from_revisions[0], to_revisions[-1],

and that's it. But we can also address, and perhaps this should be a separate commit, and maybe even where we move from 0.8.7 to 0.9 (leaning towards just a separate commit, downgrades are not as super critical here), is that we can get the downgrade to not even bother downgrading 68f in this case, because our target is 15ac, so 68f can still remain. A small tweak to the revision mechanics allows for that. It also reveals a kind of bug in the display of current. In current 0.8.6, if we upgrade from base to 68f, it applies through 15ac. But if we do "alembic current", it only shows 68f. It doesn't show "15ac", even though this is a head, because of the dependency. So the current gerrit has changes for these things too.

[sqlalchemy-bot]

cloew (@cloew) wrote:

Awesome! That patch fixed everything we needed.
The additional stuff sounds cool as well.

Not downgrading 68f seems like a really good idea.
I remember when I started playing with feature branches that I sometimes had odd issues with it occasionally downgrading along a branch I didn't intend.
It sounds like that would fix it.

Thanks again for the quick fix/feedback/awesomeness!

[sqlalchemy-bot]

Michael Bayer (@zzzeek) wrote:

Detect downgrades over dependencies distinctly from unmerge

Previously, a downgrade to a version that is also a dependency
to another branch, where that branch is advanced beyond
the target, would fail to be downgraded, as this would
be detected as an "umerge" even though the target version
to be INSERTed would not be present.
The patch replaces the existing heuristic
that checks for "delete" with a new one that calculates
a potential "unmerge" fully, and returns False only if we in
fact could do an unmerge.

Also change the string display of a version so that we don't
display misleading target versions that might not actually
be getting invoked.

Fixes: #377
Change-Id: I7420dd7adbd9ccf9ca85b56d9a792a85c40f3454

→ 111b1c1

[sqlalchemy-bot]

Changes by Michael Bayer (@zzzeek):

• changed status to closed