Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: add catalog.Mutation interface for table mutations #60695

Merged
merged 1 commit into from
Mar 12, 2021

Conversation

postamar
Copy link
Contributor

@postamar postamar commented Feb 17, 2021

Previously, catalog.TableDescriptor had a method, GetMutations, which
returned a slice of descpb.DescriptorMutation structs, which in turn
contain descpb.IndexDescriptor and descpb.ColumnDescriptor types. This
is an obstacle to our ongoing effort to virtualize column and index
descriptors.

This commit therefore wraps descpb.DescriptorMutation in an interface
type, catalog.Mutation, and also wraps the remaining mutation descriptor
field types:
- ConstraintToUpdate,
- PrimaryKeySwap,
- ComputedColumnSwap,
- MaterializedViewRefresh.

Release justification: This commit is safe for this release because it
is a low-risk refactor.

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@postamar postamar force-pushed the add-mutation-interface branch 2 times, most recently from 846f1eb to 346f743 Compare February 24, 2021 17:50
@postamar postamar marked this pull request as ready for review February 24, 2021 22:03
@postamar postamar requested review from a team and dt and removed request for a team February 24, 2021 22:03
Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking we wait for the branch cut on monday. I could see an argument about backports. Have not yet reviewed.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @dt)

@postamar
Copy link
Contributor Author

postamar commented Mar 4, 2021

Yes this absolutely can wait.

Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 25 of 25 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @dt)


pkg/sql/catalog/tabledesc/mutation.go, line 131 at r1 (raw file):

// AsColumn returns the corresponding Column if the mutation is on a column,
// nil otherwise.
func (m mutation) AsColumn() catalog.Column {

I look forward to the day that we have the time to really dive deeply into benchmarks to litigate the various tradeoffs of pointer methods for this struct which is gigantic and this method which cannot be inlined because it's called through an interface. That day is likely far away from now. I know it's wrong to worry about it but I can't help it sometimes.

Copy link
Contributor Author

@postamar postamar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review!

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @dt)


pkg/sql/catalog/tabledesc/mutation.go, line 131 at r1 (raw file):

Previously, ajwerner wrote…

I look forward to the day that we have the time to really dive deeply into benchmarks to litigate the various tradeoffs of pointer methods for this struct which is gigantic and this method which cannot be inlined because it's called through an interface. That day is likely far away from now. I know it's wrong to worry about it but I can't help it sometimes.

I agree, though I suspect you mistook catalog.Column with descpb.ColumnDescriptor here. In this case this method returns the former, which is backed by a small struct wrapping a pointer to a descpb.ColumnDescriptor, tl;dr AsColumn and the like effectively return pointers.

Previously, catalog.TableDescriptor had a method, GetMutations, which
returned a slice of descpb.DescriptorMutation structs, which in turn
contain descpb.IndexDescriptor and descpb.ColumnDescriptor types. This
is an obstacle to our ongoing effort to virtualize column and index
descriptors.

This commit therefore wraps descpb.DescriptorMutation in an interface
type, catalog.Mutation, and also wraps the remaining mutation descriptor
field types:
    - ConstraintToUpdate,
    - PrimaryKeySwap,
    - ComputedColumnSwap,
    - MaterializedViewRefresh.

Release note: None
@postamar
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Mar 12, 2021

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants