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: properly qualify the names of dropped views #59058

Merged
merged 1 commit into from Jan 25, 2021
Merged

sql: properly qualify the names of dropped views #59058

merged 1 commit into from Jan 25, 2021

Conversation

the-ericwang35
Copy link
Contributor

Fixes #57735.

Previously, event logs were not capturing the
fully qualified names of dropped views.
This PR changes the event logs to use the fully
qualified view names.
Tests were also updated to reflect this change.

Release note (bug fix): add qualification prefix for dropped views.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@the-ericwang35 the-ericwang35 marked this pull request as draft January 15, 2021 18:05
@the-ericwang35 the-ericwang35 marked this pull request as ready for review January 18, 2021 17:41
@the-ericwang35 the-ericwang35 requested review from ajwerner and a team and removed request for ajwerner January 18, 2021 17:41
Copy link
Contributor

@thoszhang thoszhang left a comment

Choose a reason for hiding this comment

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

Reviewed 17 of 17 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @the-ericwang35)


pkg/sql/resolver.go, line 405 at r1 (raw file):

// reverse of the Resolve() functions.
func (p *planner) getQualifiedTableName(
	ctx context.Context, desc catalog.TableDescriptor, flags tree.DatabaseLookupFlags,

I think we should just always set IncludeOffline and IncludeDropped to true and assume that the caller is fine with resolving a name for something dropped earlier in the transaction, instead of allowing callers to pass in flags. (NB: the GetImmutable methods try to resolve a leased descriptor unless AvoidCached is set. But dropped and offline descriptors can't be leased, so we just end up resolving the descriptor from the store every time. But that's fine.)

Fixes #57735.

Previously, event logs were not capturing the
fully qualified names of dropped views.
This PR changes the event logs to use the fully
qualified view names.
Tests were also updated to reflect this change.

Release note (bug fix): add qualification prefix for dropped views.
Copy link
Contributor Author

@the-ericwang35 the-ericwang35 left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @lucy-zhang)


pkg/sql/resolver.go, line 405 at r1 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

I think we should just always set IncludeOffline and IncludeDropped to true and assume that the caller is fine with resolving a name for something dropped earlier in the transaction, instead of allowing callers to pass in flags. (NB: the GetImmutable methods try to resolve a leased descriptor unless AvoidCached is set. But dropped and offline descriptors can't be leased, so we just end up resolving the descriptor from the store every time. But that's fine.)

Sure sounds good, I've changed it to always set IncludeOffline and IncludeDropped to be true

Copy link
Contributor

@thoszhang thoszhang left a comment

Choose a reason for hiding this comment

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

Reviewed 16 of 16 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

@the-ericwang35
Copy link
Contributor Author

TFTR!

bors r+

@craig
Copy link
Contributor

craig bot commented Jan 25, 2021

Build succeeded:

@craig craig bot merged commit 05a1ab6 into cockroachdb:master Jan 25, 2021
@the-ericwang35 the-ericwang35 deleted the ericwang/qualify_views branch January 25, 2021 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants