Merge cockroachdb#49585

49585: sql: use SQLInstanceID in EXPLAIN instead of OptionalNodeIDErr r=tbg a=asubiotto Tenants would previously not be able to run EXPLAIN because they wouldn't have access to a NodeID. This is now fixed by using the SQLInstanceID instead, which will provide an expected SQL-scoped identifier in a multi-tenant environment and will translate to a NodeID in a single-tenant environment. Release note: None (multitenancy-related change) Closes cockroachdb#49072 Co-authored-by: Alfonso Subiotto Marques <alfonso@cockroachlabs.com>
knz · May 28, 2020 · 96ec98f · 96ec98f
2 parents fe74c9b + 7366712
commit 96ec98f
Show file tree

Hide file tree

Showing 5 changed files with 7 additions and 18 deletions.
diff --git a/pkg/sql/explain_distsql.go b/pkg/sql/explain_distsql.go
@@ -14,7 +14,7 @@ import (
  "context"
 
  "github.com/cockroachdb/cockroach/pkg/base"
- "github.com/cockroachdb/cockroach/pkg/sql/distsql"
+ "github.com/cockroachdb/cockroach/pkg/roachpb"
  "github.com/cockroachdb/cockroach/pkg/sql/execinfrapb"
  "github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
  "github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
@@ -227,11 +227,9 @@ func (n *explainDistSQLNode) startExec(params runParams) error {
  }
  diagram.AddSpans(spans)
  } else {
- nodeID, err := params.extendedEvalCtx.NodeID.OptionalNodeIDErr(distsql.MultiTenancyIssueNo)
- if err != nil {
- return err
- }
- flows := plan.GenerateFlowSpecs(nodeID)
+ // TODO(asubiotto): This cast from SQLInstanceID to NodeID is temporary:
+ // https://github.com/cockroachdb/cockroach/issues/49596
+ flows := plan.GenerateFlowSpecs(roachpb.NodeID(params.extendedEvalCtx.NodeID.SQLInstanceID()))
  showInputTypes := n.options.Flags[tree.ExplainFlagTypes]
  diagram, err = execinfrapb.GeneratePlanDiagram(params.p.stmt.String(), flows, showInputTypes)
  if err != nil {

diff --git a/pkg/sql/explain_plan.go b/pkg/sql/explain_plan.go
@@ -19,7 +19,6 @@ import (
 
  "github.com/cockroachdb/cockroach/pkg/roachpb"
  "github.com/cockroachdb/cockroach/pkg/sql/colflow"
- "github.com/cockroachdb/cockroach/pkg/sql/distsql"
  "github.com/cockroachdb/cockroach/pkg/sql/flowinfra"
  "github.com/cockroachdb/cockroach/pkg/sql/rowexec"
  "github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
@@ -204,11 +203,9 @@ func populateExplain(
  // There might be an issue making the physical plan, but that should not
  // cause an error or panic, so swallow the error. See #40677 for example.
  distSQLPlanner.FinalizePlan(planCtx, &physicalPlan)
- nodeID, err := params.extendedEvalCtx.NodeID.OptionalNodeIDErr(distsql.MultiTenancyIssueNo)
- if err != nil {
- return err
- }
- flows := physicalPlan.GenerateFlowSpecs(nodeID)
+ // TODO(asubiotto): This cast from SQLInstanceID to NodeID is temporary:
+ // https://github.com/cockroachdb/cockroach/issues/49596
+ flows := physicalPlan.GenerateFlowSpecs(roachpb.NodeID(params.extendedEvalCtx.NodeID.SQLInstanceID()))
  flowCtx := makeFlowCtx(planCtx, physicalPlan, params)
  flowCtx.Cfg.ClusterID = &distSQLPlanner.rpcCtx.ClusterID
 

diff --git a/pkg/sql/logictest/testdata/logic_test/rename_column b/pkg/sql/logictest/testdata/logic_test/rename_column
@@ -1,7 +1,3 @@
-# This and all the rename_* variants fail due to
-# https://github.com/cockroachdb/cockroach/issues/47900. Specifically, being
-# unable to access a node ID during EXPLAIN.
-# LogicTest: !3node-tenant
 statement ok
 CREATE TABLE users (
  uid INT PRIMARY KEY,

diff --git a/pkg/sql/logictest/testdata/logic_test/rename_index b/pkg/sql/logictest/testdata/logic_test/rename_index
@@ -1,4 +1,3 @@
-# LogicTest: !3node-tenant
 statement ok
 CREATE TABLE users (
  id INT PRIMARY KEY,

diff --git a/pkg/sql/logictest/testdata/logic_test/values b/pkg/sql/logictest/testdata/logic_test/values
@@ -1,4 +1,3 @@
-# LogicTest: !3node-tenant
 # Tests for the implicit one row, zero column values operator.
 query I
 SELECT 1