sql/catalog,migrations: propagate HasPostDeserializationChanges, simplify migrations#76590
Conversation
|
This change is |
c7fbe91 to
3f5179c
Compare
postamar
left a comment
There was a problem hiding this comment.
Reviewed 16 of 20 files at r1, 9 of 9 files at r3, 5 of 5 files at r4, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @RichardJCai)
pkg/migration/migrations/descriptor_utils.go, line 74 at r4 (raw file):
func runPostDeserializationChangesOnAllDescriptors( ctx context.Context, d migration.TenantDeps, ) error {
I'm glad you're putting this thing in our migration toolbox of sorts. I feel like there's bound to be at least one such migration in each release.
pkg/sql/catalog/descriptor.go, line 215 at r3 (raw file):
GetDeclarativeSchemaChangerState() *scpb.DescriptorState // HasPostDeserializationChanges returns if the MutableDescriptor was changed after running
nit: s/MutableDescriptor/Descriptor
pkg/sql/catalog/dbdesc/database_desc.go, line 48 at r3 (raw file):
// changed represents whether or not the descriptor was changed // after RunPostDeserializationChanges. changed bool
While we're at it, consider:
- moving
PostDeserializationChangeTypetocatalogand, - store the
map[PostDeserializationChangeType]boolin thisimmutableinstead of this flag, - same with other descriptor types,
- replace
HasPostDeserializationChangeswith, say,ForEachPostDeserializationChange(func(PostDeserializationChangeType) error) error - provide
HasPostDeserializationChangesas a helper function which calls thisForEachPostDeserializationChangeinstead.
Feel free to not bother if you don't want to, & file a follow-up issue based off this comment & assign it to me.
pkg/migration/migrations/ensure_constraint_id_test.go, line 126 at r4 (raw file):
[][]string{{"false", "false"}}, ) log.Infof(ctx, "okay about to upgrade")
detritus?
…alog This is an imperfect change that feels a bit ad-hoc for my liking, but does improve the situation. The problem is that in recent changes, we no longer just retreive mutable descriptors and then sometimes clone Immutable descriptors. Instead we create the more general Immutable descriptors and clone out mutables. The problem is that when we did this, we threw away information regarding whether the descriptor itself had been modified by post-deserialization. In general, this is mostly an exercise in plumbing. The key feature is that when we retreive a Mutable descriptor and it was changed, we need to know that. This change tests that. There is some ad-hoc code to propagate isUncommitedVersion in various places which I don't feel great about, but it also felt wrong not to. A follow-up should come through to really nail down the properties here. The existence of NewBuilder and the fact that it's called in various places is another mechanism by which this information could be erased, but that's less of a concern for me. This change makes it possible to simplify migrations around descriptor upgrades. Release note: None
We have two migrations which have the goal of iterating through the descriptors and re-writing them if there are any changes due to the post-deserialization logic. They were needlessly complex and low-level. Probably part of the reason was that we were swallowing this information. This commit reworks them to use shared library code. Probably one of them could be eliminated altogether. Release note: None
3f5179c to
698b97c
Compare
ajwerner
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/catalog/dbdesc/database_desc.go, line 48 at r3 (raw file):
Previously, postamar (Marius Posta) wrote…
While we're at it, consider:
- moving
PostDeserializationChangeTypetocatalogand,- store the
map[PostDeserializationChangeType]boolin thisimmutableinstead of this flag,- same with other descriptor types,
- replace
HasPostDeserializationChangeswith, say,ForEachPostDeserializationChange(func(PostDeserializationChangeType) error) error- provide
HasPostDeserializationChangesas a helper function which calls thisForEachPostDeserializationChangeinstead.Feel free to not bother if you don't want to, & file a follow-up issue based off this comment & assign it to me.
I did this, more or less,
pkg/migration/migrations/ensure_constraint_id_test.go, line 126 at r4 (raw file):
Previously, postamar (Marius Posta) wrote…
detritus?
Done.
postamar
left a comment
There was a problem hiding this comment.
Reviewed 23 of 23 files at r5, 5 of 5 files at r6, all commit messages.
Reviewable status:
pkg/sql/catalog/dbdesc/database_desc.go, line 48 at r3 (raw file):
Previously, ajwerner wrote…
I did this, more or less,
Nicely done.
|
TFTR! bors r+ |
RichardJCai
left a comment
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status:
pkg/migration/migrations/descriptor_utils.go, line 74 at r4 (raw file):
Previously, postamar (Marius Posta) wrote…
I'm glad you're putting this thing in our migration toolbox of sorts. I feel like there's bound to be at least one such migration in each release.
Huge +1, thanks for this.
|
Build succeeded: |
This is an imperfect change that feels a bit ad-hoc for my liking, but does
improve the situation. The problem is that in recent changes, we no longer
just retrieve mutable descriptors and then sometimes clone Immutable
descriptors. Instead we create the more general Immutable descriptors and
clone out mutables. The problem is that when we did this, we threw away
information regarding whether the descriptor itself had been modified
by post-deserialization. In general, this is mostly an exercise in plumbing.
The key feature is that when we retreive a Mutable descriptor and it was
changed, we need to know that. This change tests that.
There is some ad-hoc code to propagate isUncommitedVersion in various places
which I don't feel great about, but it also felt wrong not to. A follow-up
should come through to really nail down the properties here.
The existence of NewBuilder and the fact that it's called in various places
is another mechanism by which this information could be erased, but that's
less of a concern for me. This change makes it possible to simplify migrations
around descriptor upgrades.
migrations: simplify descriptor migrations
We have two migrations which have the goal of iterating through the descriptors
and re-writing them if there are any changes due to the post-deserialization
logic. They were needlessly complex and low-level. Probably part of the reason
was that we were swallowing this information. This commit reworks them to use
shared library code.
Probably one of them could be eliminated altogether.
Release note: None