Unexpected downgrade behaviour in branch cases

[simonbowly]

A few cases of unexpected behaviour of downgrade when there are multiple current/head revisions. See #766 for tests that reproduce these issues.

Versions.

• OS: Ubuntu 20.04 LTS
• Python: 3.8.6
• Alembic: 1.4.3 (and current master branch)
• SQLAlchemy: 1.3.20
• Database: any
• DBAPI: any

1. Unexpected test case behaviour:

Expected base+2 to downgrade to revision b regardless of the branch label.

alembic/tests/test_version_traversal.py

Lines 313 to 318 in 3e178bd

	self._assert_downgrade(
	"c2branch@base+2",
	[self.d2.revision, self.d1.revision],
	[self.down_(self.d2), self.down_(self.c2), self.down_(self.d1)],
	set([self.c1.revision]),
	)

2. downgrade -1 behaviour depends on branch order:

This behaves as documented but it's difficult to determine the order in advance from the user's perspective.

Tree structure:

base -> rev1 --> rev2 (brancha)
             +-> rev2a (branchb) -> rev3a

Perhaps this should fail since the meaning of the relative revision is ambiguous?

alembic downgrade base
alembic upgrade rev3a
alembic upgrade rev2
alembic downgrade -1

=> INFO  [alembic.runtime.migration] Running downgrade rev3a -> rev2a, rev3a

alembic downgrade base
alembic upgrade rev2
alembic upgrade rev3a
alembic downgrade -1

=> INFO  [alembic.runtime.migration] Running downgrade rev2 -> rev1, rev2

3. downgrade revision-1 behaviour breaks with certain branch orders:

Runs too many revisions (one on the wrong branch) in some cases. This behaviour is dependent on upgrade order.

Tree structure:

base -> rev1 --> rev2 (brancha)
             +-> rev2a (branchb) -> rev3a

Works as expected:

alembic downgrade base
alembic upgrade rev3a
alembic upgrade rev2
alembic downgrade rev3a-1

=> INFO  [alembic.runtime.migration] Running downgrade rev3a -> rev2a, rev3a

Downgrades both branches once for same revision if upgrade are carried out in the opposite order:

alembic downgrade base
alembic upgrade rev2
alembic upgrade rev3a
alembic downgrade rev3a-1

=> INFO  [alembic.runtime.migration] Running downgrade rev2 -> rev1, rev2
=> INFO  [alembic.runtime.migration] Running downgrade rev3a -> rev2a, rev3a

[zzzeek]

I notice we are doing downgrades like ""c2branch@base+3", is this dependent on the work in #763 ?

as you're aware , downgrades are not at all very nuanced right now and especially the relative branch version stuff. if you want to commit the test cases in #766 they would have to have a @testing.fails on them , but also I would want to go through and see that they make sense. overall though it looks like we need #763 first.

[simonbowly]

Sorry I should have clarified - this is not dependent on #763. The c2branch@base+2 test was already handled in the current master branch, but to me it seems off by one in its counting.

#763 is just a patch for the merge point cases but fixing these probably involves changing the revision iteration algorithm. It seems to need some more tests beyond these few before trying that though? Not sure I'm across all the use cases that would need to be added and documented here.

Keen to help resolve some of these when you have a chance to clarify the intended behaviour - I know this isn't a high priority use case though, don't want to take up too much of your time.

[zzzeek]

Sorry I should have clarified - this is not dependent on #763. The c2branch@base+2 test was already handled in the current master branch, but to me it seems off by one in its counting.

hmm does #763 change what that identifier means (at least if it were -2). not like im opposed to that.

#763 is just a patch for the merge point cases but fixing these probably involves changing the revision iteration algorithm. It seems to need some more tests beyond these few before trying that though? Not sure I'm across all the use cases that would need to be added and documented here.

yeah it's really quite a thorny issue right now. especially downgrades which are a little more complicated for some reason.

Keen to help resolve some of these when you have a chance to clarify the intended behaviour - I know this isn't a high priority use case though, don't want to take up too much of your time.

OK do the test cases that you added need clarification as well? Usually, the people reporting these things are the ones looking very closely and can see that the system is being told, "start at X and move down 2 steps" and then it moved three steps, that's clearly wrong. if we have ambiguous cases then we should map them out and take a look. not sure when I last mentioned this but downgrades are not really a first class use case for many reasons, not just the branch navigation stuff but also real-world apps often have related data migrations that are non-reversible, etc. in openstack, downgrades are not used ever.

[simonbowly]

hmm does #763 change what that identifier means (at least if it were -2). not like im opposed to that.

It changes the -2 case in that it just downgrades one branch if this were referring to the branch point. The +2 case that I'm referring to is not changed by #763 but it seems odd because it turns:

a -> b -> c1 -> d1
       -> c2 -> d2

into

a -> b -> c1

but base+2 should surely refer to revision b and therefore result in just a -> b remaining (and the branch specification shouldn't even be needed)? If this pointed at base+3 it would make more sense but I would then also expect the downgrade to take more of a minimal approach and only touch the one branch being referred to (dropping the revision d2 in that case). Can you clarify what should happen there?

OK do the test cases that you added need clarification as well? Usually, the people reporting these things are the ones looking very closely and can see that the system is being told, "start at X and move down 2 steps" and then it moved three steps, that's clearly wrong. if we have ambiguous cases then we should map them out and take a look. not sure when I last mentioned this but downgrades are not really a first class use case for many reasons, not just the branch navigation stuff but also real-world apps often have related data migrations that are non-reversible, etc. in openstack, downgrades are not used ever.

Just (1) needs clarifying as above. (2) seems to be by design as it matches the docs (but seems ambiguous - order of downgrades is unpredictable; #763 will give a syntax to specify which head to take down). (3) is clearly a bug though?

[zzzeek]

for number one, I don't know why it's like that. I feel like there is some other set of assumptions at play, I thought it was the "implicit_base" flag thing but that does not seem to change it.

i think if we try to make the test act the way we think it should, that will inform us of what other cases pop out as no longer working, if any.

Ive spent about as much time as I have this morning to look at it and so far it seems like it should be this:

    def test_relative_downgrade(self):

        self._assert_downgrade(
            "c2branch@base+2",
            [self.d2.revision, self.d1.revision],
            [
                self.down_(self.d2),
                self.down_(self.c2),
                self.down_(self.d1),
                self.down_(self.c1),
            ],
            set([self.b.revision]),
        )

after all, if we set the dest as self.b.revision, that also does the same thing.

can we start with just that one case and see what it takes to make it work? then we might stumble upon why it's the way it is.

[zzzeek]

#789 describes more breakage with downgrades and branches. as this issue is likely becoming more critical I will try to find time to get into reworking the revisioning model if contributors are no longer available to work on this.

[simonbowly]

Hey @zzzeek, sorry I haven't got to this yet. I have some time this week to put a more PR together for these issues.

[zzzeek]

Hey @zzzeek, sorry I haven't got to this yet. I have some time this week to put a more PR together for these issues.

oh hey you're right there! sorry I thought you vanished. thanks! I think I got #789 which I think is it's own thing. all the stuff here likely all still the same if you think you might have time to work on it.

[simonbowly]

Sorry just got very busy over new years and lost track of a few things. Still keen to resolve this so I will have something ready in the next few days :)

[zzzeek]

thanks!

[sqla-tester]

Simon Bowly has proposed a fix for this issue in the master branch:

New downgrade algorithm to fix branch behaviour https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/2530