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: fix crdb_internal.{leases,node_runtime_info} when accessed by a tenant #55738

Merged
merged 1 commit into from
Oct 20, 2020
Merged

sql: fix crdb_internal.{leases,node_runtime_info} when accessed by a tenant #55738

merged 1 commit into from
Oct 20, 2020

Conversation

asubiotto
Copy link
Contributor

Previously, accessing either of theses tables would lead to an internal error.
This was caused by defaulting to using a null node ID if the node ID was
unavailable, causing a null violation during validation.

The rest of internal tables simply use the zero node ID, so this commit fixes
crdb_internal.{leases,node_runtime_info} to use this behavior as well.

Release note: None

Fixes #55701

…tenant

Previously, accessing either of theses tables would lead to an internal error.
This was caused by defaulting to using a null node ID if the node ID was
unavailable, causing a null violation during validation.

The rest of internal tables simply use the zero node ID, so this commit fixes
crdb_internal.{leases,node_runtime_info} to use this behavior as well.

Release note: None
@asubiotto asubiotto added the A-multitenancy Related to multi-tenancy label Oct 20, 2020
@asubiotto asubiotto requested a review from tbg October 20, 2020 12:43
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Member

@tbg tbg left a comment

Choose a reason for hiding this comment

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

Thank you and sorry, I think I screwed this one up.

@asubiotto
Copy link
Contributor Author

No worries! TFTR

bors r=tbg

@craig
Copy link
Contributor

craig bot commented Oct 20, 2020

Build succeeded:

@craig craig bot merged commit c0f7137 into cockroachdb:master Oct 20, 2020
# LogicTest: 3node-tenant

query II
SELECT count(distinct(node_id)), count(*) FROM crdb_internal.node_runtime_info
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be good to have at least one test case for all the internal tables and functions, to ensure they don't regress. I'd copy the entire crdb_internal suite, and then modify it for tenants, so that our testing is at least as good as that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Doing this in #55860 and will backport both these commits to 20.2.1

@andy-kimball
Copy link
Contributor

Thanks for fixing this so quickly. Can you be sure to backport it to 20.2.1?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-multitenancy Related to multi-tenancy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tenants: internal error when selecting from crdb_internal.node_runtime_info
5 participants