From 28a6abcbdbed866ada879e4c331c38d157481663 Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Wed, 21 Dec 2022 12:26:38 +0100 Subject: [PATCH 1/2] sql: change `RequireActiveIndex` to `IncludeNonActiveIndex` The default value of a `bool` field should encode the common case. The common case for index resolution should be to include only active indexes. So this patch changes `RequireActiveIndex` (`false` is non-common case) to `IncludeNonActiveIndex` (`false` becomes common case). Release note: None --- pkg/sql/catalog/resolver/resolver.go | 10 +++++----- pkg/sql/catalog/resolver/resolver_test.go | 22 +++++++++++----------- pkg/sql/opt_catalog.go | 6 +++--- pkg/sql/resolver.go | 7 ++++--- pkg/sql/schemachanger/scdeps/build_deps.go | 2 +- pkg/sql/sem/tree/name_resolution.go | 13 ++++++++----- 6 files changed, 32 insertions(+), 28 deletions(-) diff --git a/pkg/sql/catalog/resolver/resolver.go b/pkg/sql/catalog/resolver/resolver.go index 8735e2f4a3fb..5700b8a03a6a 100644 --- a/pkg/sql/catalog/resolver/resolver.go +++ b/pkg/sql/catalog/resolver/resolver.go @@ -523,7 +523,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 +548,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 +591,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 +702,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 +732,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 index a8cdf0e9a8cb..5726abe3e996 100644 --- 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_catalog.go b/pkg/sql/opt_catalog.go index 0b8d646fa67b..8505c178f512 100644 --- 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: false, + IncludeOfflineTable: flags.IncludeOfflineTables, }, ) }) diff --git a/pkg/sql/resolver.go b/pkg/sql/resolver.go index 6170ced0d3af..71535cba239a 100644 --- 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 index b37ec10e52b8..e989c05d6071 100644 --- 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 index 594addbfb85b..d82ec5fc3227 100644 --- 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 } From 8d823f578356535708af6d6093dd2af92c8b1e1d Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Wed, 21 Dec 2022 12:59:55 +0100 Subject: [PATCH 2/2] catalog: properly propagate flags Needed for #93644. Without this patch, it's still not possible to look up offline tables and non-active indexes. Release note: None --- pkg/sql/catalog/resolver/resolver.go | 5 ++++- pkg/sql/opt/cat/catalog.go | 6 ++++++ pkg/sql/opt_catalog.go | 2 +- 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/pkg/sql/catalog/resolver/resolver.go b/pkg/sql/catalog/resolver/resolver.go index 5700b8a03a6a..fbf201e9dca3 100644 --- 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, } diff --git a/pkg/sql/opt/cat/catalog.go b/pkg/sql/opt/cat/catalog.go index 3c6465392a30..8764dba68bc8 100644 --- 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 index 8505c178f512..4f072079b98e 100644 --- a/pkg/sql/opt_catalog.go +++ b/pkg/sql/opt_catalog.go @@ -264,7 +264,7 @@ func (oc *optCatalog) ResolveIndex( name, tree.IndexLookupFlags{ Required: true, - IncludeNonActiveIndex: false, + IncludeNonActiveIndex: flags.IncludeNonActiveIndexes, IncludeOfflineTable: flags.IncludeOfflineTables, }, )