sql: deprecate TableDescriptor.GCMutations#75280
sql: deprecate TableDescriptor.GCMutations#75280craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
postamar
left a comment
There was a problem hiding this comment.
Let's see what CI finds to complain about but this looks OK to me so far. I can already tell you that you'll have to remove the "GCMutations" entry from the validationMap in validate_test.go in the tabledesc package.
| repeated GCDescriptorMutation gc_mutations = 33 [(gogoproto.nullable) = false, | ||
| (gogoproto.customname) = "GCMutations"]; | ||
| (gogoproto.customname) = "GCMutations", | ||
| deprecated = true]; |
There was a problem hiding this comment.
I believe you could get away with outright removing this field (well, replacing it with a reserved 33 to prevent reuse of this tag number in this proto) and then remove updateDescriptorGCMutations outright. Sure, any descriptors might still have a populated GCMutations around, but:
- it's a handful of bytes at worst,
- if it's never read, it doesn't matter what's in it,
- plus it will disappear the next time that the table descriptor is updated.
This shouldn't cause any problems in a mixed-version cluster either. Worst case the GC job gets picked up by a node with the old version, which then edits this slice when it's done. It kind of expects the GC'd index to be in the slice, but doesn't complain if it's not.
There was a problem hiding this comment.
Happy to go either way here. What would SQL prefer?
|
There are CI failures but they should be trivial to fix. Nice! I look forward to approving this. |
ajwerner
left a comment
There was a problem hiding this comment.
Reviewed 9 of 10 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @postamar, and @stevendanna)
pkg/sql/gcjob/descriptor_utils.go, line 26 at r1 (raw file):
) // updateDescriptorGCMutations removes the GCMutation for tie given
s/tie/the/
e3d31ce to
f80672c
Compare
This appears unused. While the schema changer adds entries that the gc job subsequently removes, the only other code that made use of this field (outside of tests) was FindIndexByID. FindIndexByID appears to use it to return a special error that no one looks for. Release note: None
f80672c to
c08ae50
Compare
postamar
left a comment
There was a problem hiding this comment.
Reviewed 10 of 10 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status:
pkg/sql/catalog/descpb/structured.proto, line 1124 at r1 (raw file):
Previously, stevendanna (Steven Danna) wrote…
Happy to go either way here. What would SQL prefer?
The best PR is the PR that gets merged quickly, so please just go ahead and merge this one and we'll create ourselves a followup issue to track the removal in the next release.
|
bors r=postamar,ajwerner |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build succeeded: |
This appears unused. While the schema changer adds entries that the gc
job subsequently removes, the only other code that made use of this
field (outside of tests) was FindIndexByID. FindIndexByID appears to
use it to return a special error that no one looks for.
Release note: None