Skip to content

Commit b8f6f3b

Browse files
bghalbghal
committed
sql: use INSPECT parameters to disambiguate index names
Previously, the indexes named in the `INSPECT` command's options were resolved without using the database or table name as context. This change uses the name of the table and/or database to fill out the index name. Epic: CRDB-30356 Informs: #155056 Release note (sql change): The `INSPECT` command now uses the table or database parameter as context for resolving indexes specified in the options.
1 parent 6973f76 commit b8f6f3b

File tree

4 files changed

+258
-43
lines changed

4 files changed

+258
-43
lines changed

‎pkg/sql/inspect/inspect_plan.go‎

Lines changed: 59 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212
"github.com/cockroachdb/cockroach/pkg/jobs"
1313
"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
1414
"github.com/cockroachdb/cockroach/pkg/sql"
15+
"github.com/cockroachdb/cockroach/pkg/sql/catalog"
1516
"github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo"
1617
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
1718
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
@@ -34,6 +35,11 @@ func inspectTypeCheck(
3435

3536
// inspectRun represents the runtime state of an execution of INSPECT.
3637
type inspectRun struct {
38+
table catalog.TableDescriptor
39+
db catalog.DatabaseDescriptor
40+
41+
namedIndexes tree.TableIndexNames
42+
3743
checks []*jobspb.InspectDetails_Check
3844
asOfTimestamp hlc.Timestamp
3945
}
@@ -43,40 +49,77 @@ func newInspectRun(
4349
) (inspectRun, error) {
4450
var run inspectRun
4551

52+
switch stmt.Typ {
53+
case tree.InspectTable:
54+
if table, err := p.ResolveExistingObjectEx(ctx, stmt.Table, true /* required */, tree.ResolveRequireTableDesc); err != nil {
55+
return inspectRun{}, err
56+
} else {
57+
run.table = table
58+
}
59+
60+
if db, err := p.Descriptors().ByIDWithLeased(p.Txn()).Get().Database(ctx, run.table.GetParentID()); err != nil {
61+
return inspectRun{}, err
62+
} else {
63+
run.db = db
64+
}
65+
case tree.InspectDatabase:
66+
if db, err := p.Descriptors().ByNameWithLeased(p.Txn()).Get().Database(ctx, stmt.Database.ToUnresolvedName().String()); err != nil {
67+
return inspectRun{}, err
68+
} else {
69+
run.db = db
70+
}
71+
default:
72+
return inspectRun{}, errors.AssertionFailedf("unexpected INSPECT type received, got: %v", stmt.Typ)
73+
}
74+
4675
if len(stmt.Options) == 0 || stmt.Options.HasIndexAll() {
4776
// No options or INDEX ALL specified - inspect all indexes.
48-
4977
switch stmt.Typ {
5078
case tree.InspectTable:
51-
table, err := p.ResolveExistingObjectEx(ctx, stmt.Table, true /* required */, tree.ResolveRequireTableDesc)
79+
checks, err := sql.InspectChecksForTable(ctx, p, run.table)
5280
if err != nil {
5381
return inspectRun{}, err
5482
}
55-
56-
run.checks, err = sql.InspectChecksForTable(ctx, p, table)
57-
if err != nil {
83+
run.checks = checks
84+
case tree.InspectDatabase:
85+
if checks, err := sql.InspectChecksForDatabase(ctx, p, run.db); err != nil {
5886
return inspectRun{}, err
87+
} else {
88+
run.checks = checks
5989
}
60-
case tree.InspectDatabase:
61-
db, err := p.Descriptors().ByName(p.Txn()).Get().Database(ctx, stmt.Database.ToUnresolvedName().String())
90+
default:
91+
return inspectRun{}, errors.AssertionFailedf("unexpected INSPECT type received, got: %v", stmt.Typ)
92+
}
93+
} else {
94+
// Named indexes specified.
95+
switch stmt.Typ {
96+
case tree.InspectTable:
97+
schema, err := p.Descriptors().ByIDWithLeased(p.Txn()).Get().Schema(ctx, run.table.GetParentSchemaID())
6298
if err != nil {
6399
return inspectRun{}, err
64100
}
65101

66-
run.checks, err = sql.InspectChecksForDatabase(ctx, p, db)
67-
if err != nil {
102+
tableName := tree.MakeTableNameWithSchema(
103+
tree.Name(run.db.GetName()), tree.Name(schema.GetName()), tree.Name(run.table.GetName()),
104+
)
105+
if namedIndexes, err := stmt.Options.WithNamedIndexesOnTable(&tableName); err != nil {
68106
return inspectRun{}, err
107+
} else {
108+
run.namedIndexes = namedIndexes
109+
}
110+
case tree.InspectDatabase:
111+
if namedIndexes, err := stmt.Options.WithNamedIndexesInDatabase(run.db.GetName()); err != nil {
112+
return inspectRun{}, err
113+
} else {
114+
run.namedIndexes = namedIndexes
69115
}
70-
default:
71-
return inspectRun{}, errors.AssertionFailedf("unexpected INSPECT type received, got: %v", stmt.Typ)
72116
}
73-
} else {
74-
// Named indexes specified.
75-
checks, err := sql.InspectChecksByIndexNames(ctx, p, stmt.Options.NamedIndexes())
76-
if err != nil {
117+
118+
if checks, err := sql.InspectChecksByIndexNames(ctx, p, run.namedIndexes); err != nil {
77119
return inspectRun{}, err
120+
} else {
121+
run.checks = checks
78122
}
79-
run.checks = checks
80123
}
81124

82125
if stmt.AsOf.Expr != nil {

‎pkg/sql/inspect_job.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ func TriggerInspectJob(
104104
func InspectChecksForDatabase(
105105
ctx context.Context, p PlanHookState, db catalog.DatabaseDescriptor,
106106
) ([]*jobspb.InspectDetails_Check, error) {
107-
tables, err := p.Descriptors().ByName(p.Txn()).Get().GetAllTablesInDatabase(ctx, p.Txn(), db)
107+
tables, err := p.Descriptors().ByNameWithLeased(p.Txn()).Get().GetAllTablesInDatabase(ctx, p.Txn(), db)
108108
if err != nil {
109109
return nil, err
110110
}

‎pkg/sql/logictest/testdata/logic_test/inspect‎

Lines changed: 134 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -106,14 +106,10 @@ statement ok
106106
CREATE INDEX idx_c1 ON bar (c1);
107107
CREATE INDEX idx_c3 ON bar (c3);
108108

109-
statement error pq: index "bar@idx_c1" does not belong to table "foo"
109+
statement error pq: index "bar@idx_c1" is not on table "test.public.foo"
110110
INSPECT TABLE foo WITH OPTIONS INDEX (bar@idx_c1);
111111

112-
# TODO(148365): The table name should be used to disambiguate.
113-
statement error index name "idx_c1" is ambiguous \(found in test.public.foo and test.public.bar\)
114-
INSPECT TABLE bar WITH OPTIONS INDEX (idx_c1);
115-
116-
statement error index "dbfake.foo.idx_c1" does not belong to database "test"
112+
statement error pq: index "dbfake.foo.idx_c1" is not in database "test"
117113
INSPECT DATABASE test WITH OPTIONS INDEX (dbfake.foo.idx_c1);
118114

119115
statement error pq: index name "idx_c1" is ambiguous \(found in test.public.foo and test.public.bar\)
@@ -221,3 +217,135 @@ statement ok
221217
DROP TABLE t2;
222218

223219
subtest end
220+
221+
subtest database_resolve_indexes
222+
223+
statement ok
224+
CREATE DATABASE dbfoo;
225+
CREATE TABLE dbfoo.public.t2 (c1 INT, INDEX idx_c1 (c1));
226+
CREATE TABLE dbfoo.public.t1 (c1 INT, INDEX idx_c1 (c1));
227+
228+
229+
statement ok
230+
SET enable_inspect_command = true;
231+
232+
statement ok
233+
INSPECT DATABASE dbfoo WITH OPTIONS INDEX (t1@idx_c1);
234+
235+
statement ok
236+
INSPECT DATABASE dbfoo WITH OPTIONS INDEX (t2@idx_c1);
237+
238+
statement ok
239+
INSPECT DATABASE dbfoo WITH OPTIONS INDEX (public.t1@idx_c1);
240+
241+
statement ok
242+
INSPECT DATABASE dbfoo WITH OPTIONS INDEX (dbfoo.t1@idx_c1);
243+
244+
statement ok
245+
INSPECT DATABASE dbfoo WITH OPTIONS INDEX (dbfoo.t2@idx_c1);
246+
247+
statement ok
248+
INSPECT DATABASE dbfoo WITH OPTIONS INDEX (dbfoo.public.t1@idx_c1);
249+
250+
statement error pq: index name "idx_c1" is ambiguous \(found in dbfoo.public.t2 and dbfoo.public.t1\)
251+
INSPECT DATABASE dbfoo WITH OPTIONS INDEX (idx_c1);
252+
253+
statement error pq: index name "idx_c1" is ambiguous \(found in dbfoo.public.t2 and dbfoo.public.t1\)
254+
INSPECT DATABASE dbfoo WITH OPTIONS INDEX (public.idx_c1);
255+
256+
statement error pq: index name "idx_c1" is ambiguous \(found in dbfoo.public.t2 and dbfoo.public.t1\)
257+
INSPECT DATABASE dbfoo WITH OPTIONS INDEX (dbfoo.idx_c1);
258+
259+
statement error pq: index name "idx_c1" is ambiguous \(found in dbfoo.public.t2 and dbfoo.public.t1\)
260+
INSPECT DATABASE dbfoo WITH OPTIONS INDEX (dbfoo.public.idx_c1);
261+
262+
statement ok
263+
DROP DATABASE dbfoo;
264+
265+
subtest schema_catalog_collision
266+
267+
statement ok
268+
CREATE DATABASE ambiguous;
269+
CREATE SCHEMA ambiguous.ambiguous;
270+
CREATE TABLE ambiguous.ambiguous.t1 (c1 INT, INDEX idx_c1 (c1));
271+
272+
# The duplicated names of the schema and the catalog means the database parameter doesn't help with resolution.
273+
statement error pq: index "idx_c1" does not exist
274+
INSPECT DATABASE ambiguous WITH OPTIONS INDEX (ambiguous.idx_c1);
275+
276+
statement ok
277+
DROP DATABASE ambiguous;
278+
subtest end
279+
280+
subtest end
281+
282+
subtest table_resolve_indexes
283+
284+
statement ok
285+
CREATE DATABASE dbfoo;
286+
CREATE DATABASE dbbar;
287+
CREATE SCHEMA dbfoo.s1;
288+
CREATE TABLE dbfoo.public.t1 (c1 INT, INDEX idx_c1 (c1));
289+
CREATE TABLE dbfoo.s1.t1 (c1 INT, c2 INT, INDEX idx_c1 (c1), INDEX idx_c2 (c2));
290+
CREATE TABLE dbbar.public.t1 (c1 INT, c2 INT, INDEX idx_c1 (c1), INDEX idx_c2 (c2));
291+
CREATE TABLE dbbar.public.t2 (c1 INT, c2 INT, INDEX idx_c1 (c1), INDEX idx_c2 (c2));
292+
293+
statement ok
294+
SET enable_inspect_command = true;
295+
296+
subtest all_forms
297+
298+
statement ok
299+
INSPECT TABLE dbfoo.t1 WITH OPTIONS INDEX (t1@idx_c1);
300+
301+
statement error pq: index "s1.t1@idx_c1" is not on table "dbfoo.public.t1"
302+
INSPECT TABLE dbfoo.t1 WITH OPTIONS INDEX (s1.t1@idx_c1);
303+
304+
statement ok
305+
INSPECT TABLE dbfoo.t1 WITH OPTIONS INDEX (dbfoo.t1@idx_c1);
306+
307+
statement error pq: index "dbfoo.s1.t1@idx_c1" is not on table "dbfoo.public.t1"
308+
INSPECT TABLE dbfoo.t1 WITH OPTIONS INDEX (dbfoo.s1.t1@idx_c1);
309+
310+
statement ok
311+
INSPECT TABLE dbfoo.t1 WITH OPTIONS INDEX (idx_c1);
312+
313+
statement error pq: index "s1.idx_c1" is not on table "dbfoo.public.t1"
314+
INSPECT TABLE dbfoo.t1 WITH OPTIONS INDEX (s1.idx_c1);
315+
316+
statement ok
317+
INSPECT TABLE dbfoo.t1 WITH OPTIONS INDEX (dbfoo.idx_c1);
318+
319+
statement error pq: index "dbfoo.s1.idx_c1" is not on table "dbfoo.public.t1"
320+
INSPECT TABLE dbfoo.t1 WITH OPTIONS INDEX (dbfoo.s1.idx_c1);
321+
322+
statement ok
323+
INSPECT TABLE dbfoo.s1.t1 WITH OPTIONS INDEX (t1@idx_c1);
324+
325+
statement ok
326+
INSPECT TABLE dbfoo.s1.t1 WITH OPTIONS INDEX (s1.t1@idx_c1);
327+
328+
statement ok
329+
INSPECT TABLE dbfoo.s1.t1 WITH OPTIONS INDEX (dbfoo.t1@idx_c1);
330+
331+
statement ok
332+
INSPECT TABLE dbfoo.s1.t1 WITH OPTIONS INDEX (dbfoo.s1.t1@idx_c1);
333+
334+
statement ok
335+
INSPECT TABLE dbfoo.s1.t1 WITH OPTIONS INDEX (idx_c1);
336+
337+
statement ok
338+
INSPECT TABLE dbfoo.s1.t1 WITH OPTIONS INDEX (s1.idx_c1);
339+
340+
statement ok
341+
INSPECT TABLE dbfoo.s1.t1 WITH OPTIONS INDEX (dbfoo.idx_c1);
342+
343+
statement ok
344+
INSPECT TABLE dbfoo.s1.t1 WITH OPTIONS INDEX (dbfoo.s1.idx_c1);
345+
346+
subtest end
347+
348+
statement ok
349+
DROP DATABASE dbfoo;
350+
351+
subtest end

‎pkg/sql/sem/tree/inspect.go‎

Lines changed: 64 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -61,18 +61,79 @@ func (n *Inspect) Format(ctx *FmtCtx) {
6161
// InspectOptions corresponds to a comma-delimited list of inspect options.
6262
type InspectOptions []InspectOption
6363

64-
// NamedIndexes flattens the indexes named by option.
65-
func (n *InspectOptions) NamedIndexes() TableIndexNames {
64+
// namedIndexes flattens and copies the indexes named by option.
65+
func (n *InspectOptions) namedIndexes() TableIndexNames {
6666
var names TableIndexNames
6767
for _, option := range *n {
6868
if opt, ok := option.(*InspectOptionIndex); ok {
69-
names = append(names, opt.IndexNames...)
69+
for _, n := range opt.IndexNames {
70+
name := *n
71+
names = append(names, &name)
72+
}
7073
}
7174
}
7275

7376
return names
7477
}
7578

79+
// SetNamedIndexesToTable returns a copy of the named indexes with each table
80+
// set to the specified 3-part name. It errors if the existing table name can't
81+
// be qualified to the new table name.
82+
func (n *InspectOptions) WithNamedIndexesOnTable(tableName *TableName) (TableIndexNames, error) {
83+
tabledIndexes := n.namedIndexes()
84+
for _, index := range tabledIndexes {
85+
if !indexMatchesTable(index, *tableName) {
86+
return nil, pgerror.Newf(pgcode.InvalidName, "index %q is not on table %q", index.String(), tableName)
87+
}
88+
index.Table = *tableName
89+
}
90+
return tabledIndexes, nil
91+
}
92+
93+
// WithNamedIndexesInDatabase returns a copy of the named indexes with each
94+
// table name set to include database specified. It errors if the database was
95+
// previously set and is not a match.
96+
//
97+
// In 2-part names, the schema field is used ambiguously for either the database
98+
// or the schema. To prevent duplication of the database name, the catalog field
99+
// isn't set if the database name is the same as the schema name.
100+
func (n *InspectOptions) WithNamedIndexesInDatabase(databaseName string) (TableIndexNames, error) {
101+
databasedIndexes := n.namedIndexes()
102+
for _, index := range databasedIndexes {
103+
if index.Table.ExplicitCatalog && index.Table.CatalogName != Name(databaseName) {
104+
return nil, pgerror.Newf(pgcode.InvalidName, "index %q is not in database %q", index.String(), databaseName)
105+
}
106+
107+
if index.Table.ExplicitSchema {
108+
if index.Table.SchemaName != Name(databaseName) {
109+
index.Table.CatalogName = Name(databaseName)
110+
index.Table.ExplicitCatalog = true
111+
}
112+
} else {
113+
index.Table.SchemaName = Name(databaseName)
114+
index.Table.ExplicitSchema = true
115+
}
116+
}
117+
118+
return databasedIndexes, nil
119+
}
120+
121+
// indexMatchesTable checks if a TableIndexName matches a 3-part table name.
122+
func indexMatchesTable(index *TableIndexName, table TableName) bool {
123+
if index.Table.ObjectName != "" && table.ObjectName != index.Table.ObjectName {
124+
return false
125+
}
126+
if index.Table.ExplicitCatalog {
127+
return table.CatalogName == index.Table.CatalogName && table.SchemaName == index.Table.SchemaName
128+
}
129+
if index.Table.ExplicitSchema {
130+
// A 2-part name means the first segment may be the schema or the catalog.
131+
return table.CatalogName == index.Table.SchemaName || table.SchemaName == index.Table.SchemaName
132+
}
133+
134+
return true
135+
}
136+
76137
// HasIndexAll checks if the options include an INDEX ALL option.
77138
func (n *InspectOptions) HasIndexAll() bool {
78139
for _, option := range *n {
@@ -89,23 +150,6 @@ func (n *Inspect) Validate() error {
89150
return err
90151
}
91152

92-
// TODO(155056): Better validate index names from the options with the name
93-
// of the database or table from the command.
94-
// TODO(148365): Check for duplicated index names (including the name of the
95-
// database or table from the command).
96-
for _, index := range n.Options.NamedIndexes() {
97-
switch n.Typ {
98-
case InspectTable:
99-
if index.Table.ObjectName != "" && n.Table.Object() != index.Table.Object() {
100-
return pgerror.Newf(pgcode.InvalidName, "index %q does not belong to table %q", index.String(), n.Table.String())
101-
}
102-
case InspectDatabase:
103-
if index.Table.ExplicitCatalog && n.Database.Object() != index.Table.Catalog() {
104-
return pgerror.Newf(pgcode.InvalidName, "index %q does not belong to database %q", index.String(), n.Database.String())
105-
}
106-
}
107-
}
108-
109153
return nil
110154
}
111155

0 commit comments

Comments
 (0)