-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
sql/schemachanger: remove cycle DepEdge rules, add SameStage kind #73290
sql/schemachanger: remove cycle DepEdge rules, add SameStage kind #73290
Conversation
Release note: None
Just pulling symbols into the test and making the table a bit more explicit. Release note: None
Release note: None
Release note: None
Release note: None
This commit seeks to rectify an early mistake in the architecture of the declarative schema changer. In the original design, we knew we wanted certain transitions to happen in the same stage. In order to deal with that, we created rules that allowed for special types of cycles in dependencies to exist. This was a mistake. Instead, we replace this by a `Kind` property of `DepEdge`s which indicates whether the target pointed to merely needs to `HappenBefore` or whether it also needs to happen in the `SameStage`. This allows us to express exactly what we meant. This change also uncovered some broken cycles which never were intended to exist. The resultant plans generally look better. Release note: None
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo making the DepEdgeKind show up in scgraphviz
renderings. Perhaps use a double-headed arrow for those edges?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 6 of 6 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, 3 of 3 files at r4, 2 of 2 files at r5, 19 of 19 files at r6, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner)
TFTR bors r+ |
Build succeeded: |
First few commits are mostly minor cleanup. Last commit is the meat of the change.
This commit seeks to rectify an early mistake in the architecture of the
declarative schema changer. In the original design, we knew we wanted certain
transitions to happen in the same stage. In order to deal with that, we created
rules that allowed for special types of cycles in dependencies to exist. This
was a mistake. Instead, we replace this by a
Kind
property ofDepEdge
swhich indicates whether the target pointed to merely needs to
HappenBefore
or whether it also needs to happen in the
SameStage
. This allows us toexpress exactly what we meant.
This change also uncovered some broken cycles which never were intended to
exist. The resultant plans generally look better.
Release note: None