scplan: break out stage building into scstage package and invert rule directions#73674
scplan: break out stage building into scstage package and invert rule directions#73674craig[bot] merged 6 commits intocockroachdb:masterfrom
Conversation
|
This change is |
|
This is best reviewed commit by commit. The first two are strict refactors, the third is tiny, and the fourth is dominated by test artifact noise. Ultimately, this PR doesn't really change anything substantial.
edit: I've since pushed more commits to this branch, because the strict precedence change was much less disruptive than I feared. In the process I found a couple more bugs to fix, and I added extra validation to check that each stage is internally consistent and so that I can trust what I see when look at the output of |
4acae86 to
1d57d08
Compare
ajwerner
left a comment
There was a problem hiding this comment.
This is Good. One concern about a behavior change. Otherwise,
Reviewed 9 of 9 files at r1, 1 of 1 files at r2, 3 of 3 files at r3, 13 of 14 files at r4, 1 of 1 files at r5, 1 of 1 files at r6, 3 of 3 files at r7, 17 of 17 files at r8.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @postamar)
pkg/sql/schemachanger/scplan/opgen/op_gen.go, line 46 at r6 (raw file):
edgesToAdd = edgesToAdd[:0] if err := t.iterateFunc(g.Database(), func(n *scpb.Node) error { status := n.Status
I recall this having something to do with edge cases involving nodes which were referenced by dependencies. I think this was vestigial. Thanks for cleaning it up. Cleanup passes from me around some of this code are overdue.
pkg/sql/schemachanger/scplan/scstage/stage.go, line 27 at r1 (raw file):
// the stage, reflecting the fact that any set of ops can be thought of as a // transition from one state to another. type Stage struct {
I could see moving this to scgraph. Ultimately we'll want the visualization stuff to have awareness of this.
pkg/sql/schemachanger/scplan/scstage/stage.go, line 114 at r9 (raw file):
revertibleAllowed := true for _, stage := range stages {
Can we add a check that there is at most one PreCommit stage?
pkg/sql/schemachanger/testdata/alter_table_add_column, line 238 at r8 (raw file):
Quoted 17 lines of code…
update progress of schema change job #1 upsert descriptor #56 ... version: 4 mutationId: 2 - state: DELETE_AND_WRITE_ONLY + state: DELETE_ONLY name: tbl newSchemaChangeJobId: "1" ... time: {} unexposedParentSchemaId: 55 - version: "4" + version: "5" commit transaction #8 begin transaction #9 ## PostCommitPhase non-revertible stage 8 of 8 with 3 MutationType ops
why did this stage go away? It seems important
postamar
left a comment
There was a problem hiding this comment.
Thanks for the review. I'll salvage as much as I can from this PR and rework the strict precedence + collapse.
Reviewable status:
pkg/sql/schemachanger/scplan/scstage/stage.go, line 27 at r1 (raw file):
Previously, ajwerner wrote…
I could see moving this to
scgraph. Ultimately we'll want the visualization stuff to have awareness of this.
I agree in the longer term.
pkg/sql/schemachanger/scplan/scstage/stage.go, line 114 at r9 (raw file):
Previously, ajwerner wrote…
Can we add a check that there is at most one
PreCommitstage?
Yes! Let's.
pkg/sql/schemachanger/testdata/alter_table_add_column, line 238 at r8 (raw file):
Previously, ajwerner wrote…
update progress of schema change job #1 upsert descriptor #56 ... version: 4 mutationId: 2 - state: DELETE_AND_WRITE_ONLY + state: DELETE_ONLY name: tbl newSchemaChangeJobId: "1" ... time: {} unexposedParentSchemaId: 55 - version: "4" + version: "5" commit transaction #8 begin transaction #9 ## PostCommitPhase non-revertible stage 8 of 8 with 3 MutationType opswhy did this stage go away? It seems important
Oh, good catch. My changes basically violate the two version invariant, which is bad!
What's happening is that a bunch of stages got collapsed together when they shouldn't have. It seems that we were relying on the fact that each state transition was a separate edge and therefore (pre-this-PR at least) a separate stage.
In retrospect this strict-precedence change needs some adjusting. I'm not such a huge fan of violating it after collapsing the stages. In fact, knowing what I know now, I'd feel easier if the two-version invariant were enforced by the rules engine somehow.
I'm thinking now that perhaps it might be best to separate precedence constraints from same-stage/different-stage constraints somehow. I'll go back to the drawing board.
Also this violation should have been caught by the stage validation checks.
This commit is strictly a refactor which moves all stage planning logic from the declarative schema changer into its own package, with no change to the structure of the code or the logic in the function bodies. Release note: None
This commit breaks down the logic that was previously inside the BuildStages function's body into separate functions. Mainly, the closures have been made into methods on a buildCtx struct which basically is the closure's captured variables made explicit. This commit is strictly a refactor. Release note: None
This change was motivated by the upcoming need to introduce depEdgesTo to complement the existing depEdgesFrom field in the graph struct. Release note: None
This commit redefines the declarative schema changer's dependency rules as precedence rules instead of succession rules. This has been found to be more intuitive to reason about. It also makes for better graphviz representations. Release note: None
The existing graph builder logic had a bug where too many op edges were added to the graph. This caused additional source nodes to appear which were not in the initial state. This bug was probably harmless but made for some confusing dependency graph renderings. Release note: None
This commit fixes a few things in the declarative schema changer's visualization package: - Same-state precedence dep edges have diamond arrow heads to distinguish them from regular precedence edges. - A bug was causing some ops to not be visible in the stage rendering. - Target counters are left-padded with zeros because graphviz orders the graph nodes by lexicographical order. They now appear in the same order as in the schema changer state. - The schema changer job ops are now visible in the stage rendering. They were previously not rendered, which made the ops count in the stage label inconsistent. Release note: None
|
I force-pushed this branch to keep only the safe and uncontroversial commits at the beginning of it, which I will bors as soon as CI gives a green light. The removed commits are still being reworked. |
|
bors r+ |
|
Build succeeded: |
These 4 commits introduce a new scstage package. This was motivated by the more immediate goal of inverting dependency edge directions, to express them as precedence constraints instead of succession constraints.