sql/schemachanger: add compatibility with 22.2 rules #95361
Conversation
|
This change is |
fqazi
force-pushed
the
ruleBackCompat
branch
2 times, most recently
from
3aaf70a to
87ce2fe
Compare
|
This is best looked at on a commit-by-commit basis, the first one just shifts things around. The second one adds 22.2 rules, there are some changes here to facilitate planning on the master, specifically for dropping constraints (which are harmless and should be optimized out general DROP cases) |
|
Also, this might be a good use case for a |
Good point, I'll eliminate the common package and move everything up |
The linter doesn't like the idea of those imports :( |
fqazi
force-pushed
the
ruleBackCompat
branch
2 times, most recently
from
07b4df3 to
df09ee9
Compare
|
@ajwerner Made both of those changes |
pkg/sql/schemachanger/scplan/plan.go
Outdated
| func applyOpRules( | ||
| ctx context.Context, activeVersion clusterversion.ClusterVersion, g *scgraph.Graph, | ||
| ) (*scgraph.Graph, error) { | ||
| if activeVersion.IsActive(clusterversion.V23_1) { | ||
| return current.ApplyOpRules(ctx, g) | ||
| } else if activeVersion.IsActive(clusterversion.V22_2) { | ||
| return release_22_2.ApplyOpRules(ctx, g) | ||
| } else { | ||
| log.Warningf(ctx, "Falling back to the oldest supported version 22.2") | ||
| return release_22_2.ApplyOpRules(ctx, g) | ||
| } | ||
| } | ||
|
|
||
| func applyDepRules( | ||
| ctx context.Context, activeVersion clusterversion.ClusterVersion, g *scgraph.Graph, | ||
| ) error { | ||
| if activeVersion.IsActive(clusterversion.V23_1) { | ||
| return current.ApplyDepRules(ctx, g) | ||
| } else if activeVersion.IsActive(clusterversion.V22_2) { | ||
| return release_22_2.ApplyDepRules(ctx, g) | ||
| } else { | ||
| log.Warningf(ctx, "Falling back to the oldest supported version 22.2") | ||
| return release_22_2.ApplyDepRules(ctx, g) | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: make a function to look up the registry for the version rather than dispatching each of these methods.
Also, that function is a great place to write a comment explaining why we're doing this.
There was a problem hiding this comment.
Good point, added a new function and use that here:
// getRulesRegistryForRelease returns the rules registry based on the current
// active version of cockroach. In a mixed version state, it's possible for the state
// generated by a newer version of cockroach to be incompatible with old versions.
// For example dependent objects or combinations of them in a partially executed,
// plan may reach states where older versions of cockroach may not be able
// to plan further (and vice versa). To eliminate the possibility of these issues, we
// will plan with the set of rules belonging to the currently active version. One
// example of this is the dependency between index name and secondary indexes
// is more relaxed on 23.1 vs 22.2, which can lead to scenarios where the index
// name may become public before the index is public (which was disallowed on older
// versions).
…eleases Previously, the declarative schema changer rules were tightly coupled, so we had no way of allowing them to be portal between releases for mixed version compatibility. To address this, this patch introduces a common package that will help facilitate making rules more portable between different version. Release note: None Epic: None
Previously, when rules were relaxed on master we could have compatibility issues in mixed version environments. This was inadequate because, the relaxed rules could not generate plans that could pass assertions in a mixed version state. To address this, this patch adds the 22.2 rules on master, and uses them in mixed version scenarios. Release note: none Epic: none
|
bors r+ Failure is a known test flake |
|
Build succeeded: |
This PR will make the following changes: