scbuild, rules: rewrite index handling in scbuild, fix bugs #86571
Conversation
|
This change is |
| // recreateAllSecondaryIndexes recreates all secondary indexes. While the key | ||
| // columns remain the same in the face of a primary key change, the key suffix | ||
| // columns or the stored columns may not. | ||
| func recreateAllSecondaryIndexes( | ||
| b BuildCtx, tbl *scpb.Table, newPrimaryIndex, sourcePrimaryIndex *scpb.PrimaryIndex, | ||
| ) { | ||
| // TODO(postamar): implement in 23.1 | ||
| } |
There was a problem hiding this comment.
doesn't this need to return a not implemented error?
There was a problem hiding this comment.
No. I added a comment to clarify.
| // makeIndexSpec constructs an indexSpec based on an existing index element. | ||
| func makeIndexSpec(b BuildCtx, tableID catid.DescID, indexID catid.IndexID) (s indexSpec) { |
There was a problem hiding this comment.
should this panic an error if it can't find the index? right now it'll just return an empty spec, no?
| // makeSwapIndexSpec constructs a pair of indexSpec for the new index and the | ||
| // accompanying temporary index to swap out an existing index with. | ||
| func makeSwapIndexSpec( |
There was a problem hiding this comment.
this code implicitly assumes that the spec is either primary or secondary, but I don't see code anywhere to assert that that's true. I'm fine with having this code assume out is well-formed, but something above it needs to assert that
| if err != nil { | ||
| return nil, err | ||
| var constraintName string | ||
| if constraint, err := tbl.FindConstraintWithID(e.ConstraintID); err != nil { |
There was a problem hiding this comment.
@Xiang-Gu were you doing something about this?
postamar
force-pushed
the
fix-constraint-and-comment-bugs
branch
from
06fdc67 to
e0df27d
Compare
|
Ready for another look. |
There was a problem hiding this comment.
Reviewed 4 of 25 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @postamar)
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_primary_key.go line 354 at r2 (raw file):
// if we drop the sharding on the primary, we need to maintain it on the // secondaries if they exist. if !hasSecondary {
Just for my own understanding, this too will always be true because of fallBackIfSecondaryIndexExists, right?
postamar
force-pushed
the
fix-constraint-and-comment-bugs
branch
from
e0df27d to
11e3a71
Compare
This commit fixes bugs related to constraints and comments in the declarative schema changer. This commit also rewrites how index targets are manipulated inside the builder, but otherwise does not change its output. This commit is a spin-off from work performed on adding secondary index support in the ALTER PRIMARY KEY implementation. That work is not going to be included in the 22.2 release, unlike this. Relates to cockroachdb#83932. Release justification: bug fixes with otherwise no functional changes Release note: None
postamar
force-pushed
the
fix-constraint-and-comment-bugs
branch
from
11e3a71 to
b88b0a5
Compare
|
bors r+ |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build succeeded: |
This commit fixes bugs related to constraints and comments in the
declarative schema changer. This commit also rewrites how index targets
are manipulated inside the builder, but otherwise does not change its
output.
This commit is a spin-off from work performed on adding secondary index
support in the ALTER PRIMARY KEY implementation. That work is not going
to be included in the 22.2 release, unlike this.
Relates to #83932.
Release justification: bug fixes with otherwise no functional changes
Release note: None