Skip to content

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

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
asubiotto:fix55701
Oct 20, 2020
Merged

sql: fix crdb_internal.{leases,node_runtime_info} when accessed by a tenant#55738
craig[bot] merged 1 commit intocockroachdb:masterfrom
asubiotto:fix55701

Conversation

@asubiotto
Copy link
Copy Markdown
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
Copy Markdown
Member

This change is Reviewable

Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

No worries! TFTR

bors r=tbg

@craig
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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