-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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: fix expandIndexName to return correct table mutable #81473
sql: fix expandIndexName to return correct table mutable #81473
Conversation
Fixes cockroachdb#81211 A regression was introduced with cockroachdb#80769 that the table mutable got created from a uncommitted descriptor. This pr tries to fix that. Release note: None.
Heh, @ecwall hit me up about this too. I had already typed something, curious to see how it makes you feel: --- a/pkg/sql/resolver.go
+++ b/pkg/sql/resolver.go
@@ -698,8 +698,24 @@ func expandIndexName(
// Memoize the table name that was found. tn is a reference to the table name
// stored in index.Table.
*tn = tableName
- tblMutable := tbl.NewBuilder().BuildExistingMutable().(*tabledesc.Mutable)
- return &tableName, tblMutable, nil
+
+ // We don't have a handle to the mutable descriptor, only the immutable. We
+ // don't know whether this is a newly created table or an existing table,
+ // so we can't build the mutable from the immutable. Instead, now that we
+ // know the right name, go resolve the corresponding mutable descriptor.
+ _, obj, err := sc.Accessor().GetObjectDesc(
+ ctx, txn, tn.Catalog(), tn.Schema(), string(tn.ObjectName), tree.ObjectLookupFlags{
+ CommonLookupFlags: tree.CommonLookupFlags{
+ Required: true,
+ RequireMutable: true,
+ },
+ DesiredObjectKind: tree.TableObject,
+ },
+ )
+ if err != nil {
+ return nil, nil, err
+ }
+ return &tableName, obj.(*tabledesc.Mutable), nil
}
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ajwerner I'm gonna leave it as it is since we both don't have strong feelings.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @chengxiong-ruan)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does some form of this need a backport to release-22.1
?
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @chengxiong-ruan)
good question. backport is not needed, because it was doing the right thing, and my refactoring did it wrong and it was not backported |
TFTR! @ajwerner |
Build failed (retrying...): |
Build succeeded: |
Fixes #81211
A regression was introduced with #80769 that the table mutable
got created from a uncommitted descriptor. This pr tries to fix
that.
Release note: None.