Merge #94048

94048: catalog: properly propagate flags r=chengxiong-ruan a=knz First commit from #94046. Needed for #93644. Without this patch, it's still not possible to look up offline tables and non-active indexes. Release note: None Epic: None Co-authored-by: Raphael 'kena' Poss <[email protected]>
cockroachdb · Dec 21, 2022 · a427001 · a427001
2 parents 7d47c0f + 8d823f5
commit a427001
Show file tree

Hide file tree

Showing 7 changed files with 42 additions and 29 deletions.
diff --git a/pkg/sql/catalog/resolver/resolver.go b/pkg/sql/catalog/resolver/resolver.go
@@ -493,7 +493,10 @@ func ResolveIndex(
 ) {
 	if tableIndexName.Table.ObjectName != "" {
 		lflags := tree.ObjectLookupFlags{
-			CommonLookupFlags:    tree.CommonLookupFlags{Required: flag.Required},
+			CommonLookupFlags: tree.CommonLookupFlags{
+				Required:       flag.Required,
+				IncludeOffline: flag.IncludeOfflineTable,
+			},
 			DesiredObjectKind:    tree.TableObject,
 			DesiredTableDescKind: tree.ResolveRequireTableOrViewDesc,
 		}
@@ -523,7 +526,7 @@ func ResolveIndex(
 
 		idx, err := candidateTbl.FindNonDropIndexWithName(string(tableIndexName.Index))
 		// err == nil indicates that the index is found.
-		if err == nil && (!flag.RequireActiveIndex || idx.Public()) {
+		if err == nil && (flag.IncludeNonActiveIndex || idx.Public()) {
 			return true, resolvedPrefix, candidateTbl, idx, nil
 		}
 
@@ -548,7 +551,7 @@ func ResolveIndex(
 		}
 
 		tblFound, tbl, idx, err := findTableContainingIndex(
-			ctx, tree.Name(tableIndexName.Index), resolvedPrefix, schemaResolver, flag.RequireActiveIndex, flag.IncludeOfflineTable,
+			ctx, tree.Name(tableIndexName.Index), resolvedPrefix, schemaResolver, flag.IncludeNonActiveIndex, flag.IncludeOfflineTable,
 		)
 		if err != nil {
 			return false, catalog.ResolvedObjectPrefix{}, nil, nil, err
@@ -591,7 +594,7 @@ func ResolveIndex(
 		schemaFound = true
 
 		candidateFound, tbl, idx, curErr := findTableContainingIndex(
-			ctx, tree.Name(tableIndexName.Index), candidateResolvedPrefix, schemaResolver, flag.RequireActiveIndex, flag.IncludeOfflineTable,
+			ctx, tree.Name(tableIndexName.Index), candidateResolvedPrefix, schemaResolver, flag.IncludeNonActiveIndex, flag.IncludeOfflineTable,
 		)
 		if curErr != nil {
 			return false, catalog.ResolvedObjectPrefix{}, nil, nil, curErr
@@ -702,7 +705,7 @@ func findTableContainingIndex(
 	indexName tree.Name,
 	resolvedPrefix catalog.ResolvedObjectPrefix,
 	schemaResolver SchemaResolver,
-	requireActiveIndex bool,
+	includeNonActiveIndex bool,
 	includeOfflineTable bool,
 ) (found bool, tblDesc catalog.TableDescriptor, idxDesc catalog.Index, err error) {
 	dsNames, _, err := schemaResolver.GetObjectNamesAndIDs(ctx, resolvedPrefix.Database, resolvedPrefix.Schema)
@@ -732,7 +735,7 @@ func findTableContainingIndex(
 
 		candidateIdx, err := candidateTbl.FindNonDropIndexWithName(string(indexName))
 		if err == nil {
-			if requireActiveIndex && !candidateIdx.Public() {
+			if !includeNonActiveIndex && !candidateIdx.Public() {
 				continue
 			}
 			if found {

diff --git a/pkg/sql/catalog/resolver/resolver_test.go b/pkg/sql/catalog/resolver/resolver_test.go
@@ -637,7 +637,7 @@ CREATE TABLE c (a INT, INDEX idx2(a));`,
 		for _, tc := range testCases {
 			t.Run(tc.testName, func(t *testing.T) {
 				_, prefix, tblDesc, idxDesc, err := resolver.ResolveIndex(
-					ctx, schemaResolver, tc.name, tree.IndexLookupFlags{Required: true, RequireActiveIndex: false})
+					ctx, schemaResolver, tc.name, tree.IndexLookupFlags{Required: true, IncludeNonActiveIndex: true})
 				var res string
 				if err != nil {
 					res = fmt.Sprintf("error: %s", err.Error())
@@ -647,7 +647,7 @@ CREATE TABLE c (a INT, INDEX idx2(a));`,
 				require.Equal(t, tc.expected, res)
 
 				_, _, _, _, err = resolver.ResolveIndex(
-					ctx, schemaResolver, tc.name, tree.IndexLookupFlags{Required: false, RequireActiveIndex: false})
+					ctx, schemaResolver, tc.name, tree.IndexLookupFlags{Required: false, IncludeNonActiveIndex: true})
 				if tc.errIfNotRequired {
 					require.Error(t, err)
 				} else {
@@ -722,9 +722,9 @@ CREATE INDEX baz_idx ON baz (s);
 			schemaResolver,
 			newTableIndexName("", "", "", "baz_idx"),
 			tree.IndexLookupFlags{
-				Required:            false,
-				RequireActiveIndex:  true,
-				IncludeOfflineTable: false,
+				Required:              false,
+				IncludeNonActiveIndex: false,
+				IncludeOfflineTable:   false,
 			},
 		)
 		require.NoError(t, err)
@@ -737,9 +737,9 @@ CREATE INDEX baz_idx ON baz (s);
 			schemaResolver,
 			newTableIndexName("", "", "", "baz_idx"),
 			tree.IndexLookupFlags{
-				Required:            true,
-				RequireActiveIndex:  false,
-				IncludeOfflineTable: true,
+				Required:              true,
+				IncludeNonActiveIndex: true,
+				IncludeOfflineTable:   true,
 			},
 		)
 		require.NoError(t, err)
@@ -753,9 +753,9 @@ CREATE INDEX baz_idx ON baz (s);
 			schemaResolver,
 			newTableIndexName("", "", "", "foo_idx"),
 			tree.IndexLookupFlags{
-				Required:            true,
-				RequireActiveIndex:  true,
-				IncludeOfflineTable: false,
+				Required:              true,
+				IncludeNonActiveIndex: false,
+				IncludeOfflineTable:   false,
 			},
 		)
 		require.NoError(t, err)

diff --git a/pkg/sql/opt/cat/catalog.go b/pkg/sql/opt/cat/catalog.go
@@ -68,6 +68,12 @@ type Flags struct {
 	// which we also want to show valid ranges when a table is being imported
 	// (offline).
 	IncludeOfflineTables bool
+
+	// IncludeNonActiveIndexes considers non-active indexes (e.g. being
+	// added). This is useful in cases where we are running a statement
+	// like `SHOW RANGES` for which we also want to show valid ranges
+	// when a table is being imported (offline).
+	IncludeNonActiveIndexes bool
 }
 
 // Catalog is an interface to a database catalog, exposing only the information

diff --git a/pkg/sql/opt_catalog.go b/pkg/sql/opt_catalog.go
@@ -263,9 +263,9 @@ func (oc *optCatalog) ResolveIndex(
 			oc.planner,
 			name,
 			tree.IndexLookupFlags{
-				Required:            true,
-				RequireActiveIndex:  true,
-				IncludeOfflineTable: flags.IncludeOfflineTables,
+				Required:              true,
+				IncludeNonActiveIndex: flags.IncludeNonActiveIndexes,
+				IncludeOfflineTable:   flags.IncludeOfflineTables,
 			},
 		)
 	})

diff --git a/pkg/sql/resolver.go b/pkg/sql/resolver.go
@@ -720,7 +720,8 @@ func expandIndexName(
 		p,
 		index,
 		tree.IndexLookupFlags{
-			Required: requireTable,
+			Required:              requireTable,
+			IncludeNonActiveIndex: true,
 		},
 	)
 	if err != nil {
@@ -768,8 +769,8 @@ func (p *planner) getTableAndIndexImpl(
 	_, resolvedPrefix, tbl, idx, err := resolver.ResolveIndex(
 		ctx, p, tableWithIndex,
 		tree.IndexLookupFlags{
-			Required:           true,
-			RequireActiveIndex: true,
+			Required:              true,
+			IncludeNonActiveIndex: false,
 		},
 	)
 	if err != nil {

diff --git a/pkg/sql/schemachanger/scdeps/build_deps.go b/pkg/sql/schemachanger/scdeps/build_deps.go
@@ -170,7 +170,7 @@ func (d *buildDeps) MayResolveIndex(
 	idx catalog.Index,
 ) {
 	found, prefix, tbl, idx, err := resolver.ResolveIndex(
-		ctx, d.schemaResolver, &tableIndexName, tree.IndexLookupFlags{},
+		ctx, d.schemaResolver, &tableIndexName, tree.IndexLookupFlags{IncludeNonActiveIndex: true},
 	)
 
 	if err != nil {

diff --git a/pkg/sql/sem/tree/name_resolution.go b/pkg/sql/sem/tree/name_resolution.go
@@ -273,11 +273,14 @@ func ObjectLookupFlagsWithRequiredTableKind(kind RequiredTableKind) ObjectLookup
 
 // IndexLookupFlags is the flag struct used for resolver.ResolveIndex() only.
 type IndexLookupFlags struct {
-	// Control if the lookup can return nil index without returning an error if
-	// the index does not exist.
+	// Required, if true, indicates lookup can return nil index without
+	// returning an error if the index does not exist.
 	Required bool
-	// Control if the lookup only considers active indexes.
-	RequireActiveIndex bool
-	// Control if the lookup considers offline tables.
+	// IncludeNonActiveIndex expands the lookup to also consider
+	// non-active indexes. By default, only active indexes are
+	// considered.
+	IncludeNonActiveIndex bool
+	// IncludeOfflineTable expands the lookup to also consider offline
+	// tables. By default, only online tables are considered.
 	IncludeOfflineTable bool
 }