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: handle no cluster_inflight_traces table on tenants #72825

Merged
merged 1 commit into from
Nov 18, 2021

Conversation

aliher1911
Copy link
Contributor

@aliher1911 aliher1911 commented Nov 16, 2021

Previously query for traces under tenant will fail with NPE.
This was happening because trace collector is not available
and virtual table was not aware of the case.
This patch adds handling of the table on tenant sql pods.

Release note: None

Fixes #72564

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Member

@RaduBerinde RaduBerinde 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 @aliher1911)


pkg/sql/crdb_internal.go, line 1293 at r1 (raw file):

		if traceCollector == nil {
			// Tenant nodes doesn't have trace collector so can't serve this table.
			return false, pgerror.New(pgcode.UndefinedTable, "table crdb_internal.cluster_inflight_traces does not exist")

The error can be confusing, especially if the table shows up in internal catalogs. I would check !planner.ExecCfg().Codec.ForSystemTenant instead and emit an error saying this is not implemented for tenants.

@aliher1911 aliher1911 force-pushed the multi_tenant_no_trace branch 2 times, most recently from 32a7262 to f610523 Compare November 17, 2021 11:25
@aliher1911 aliher1911 marked this pull request as ready for review November 17, 2021 11:26
@aliher1911
Copy link
Contributor Author

Or is it better to add the test to pkg/sql/logictest/testdata/logic_test/crdb_internal_tenant ?

Copy link
Contributor

@adityamaru adityamaru 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 fixing this!

Previously query for traces under tenant will fail with NPE.
This was happening because trace collector is not available
and virtual table was not aware of the case.
This patch adds handling of the table on tenant sql pods.

Release note: None
@aliher1911
Copy link
Contributor Author

bors r=adityamaru

@craig craig bot merged commit 4c8c0d4 into cockroachdb:master Nov 18, 2021
@craig
Copy link
Contributor

craig bot commented Nov 18, 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.

tracing: NPE in SELECT from crdb_internal.cluster_inflight_traces inside tenant
4 participants