From 1ab58310d75c1805ce35fe09bef1fefd67be0b47 Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Fri, 13 Apr 2018 10:58:57 +0200 Subject: [PATCH 1/2] sql: fix qualified index name resolution In CockroachDB index names are relative to a table name (e.g. `tbl@idx`) but in Postgres index names live in the schema namespace. To offer compatibility with Postgres, CockroachDB implements a special name resolution algorithm for index names specified without the '@' syntax. For example, `DROP INDEX foo` will search all tables in the current schema to find one with index name `foo`. Prior to this patch, CockroachDB was not able to search for a _qualified_ index name specified without the '@' syntax. For example, TypeORM issues `DROP INDEX public."primary"`, expecting to be able to drop the primary index of some table in the `public` schema. This was not recognized by CockroachDB. This patch extends the name resolution for index names to support both partial and complete qualification, using the same name resolution rules as other objects. Release note (sql change): CockroachDB now supports more ways to specify an index name for statements that require one (e.g., `DROP INDEX`, `ALTER INDEX ... RENAME`, etc.), in a way more compatible with PostgreSQL. --- pkg/ccl/partitionccl/zone_test.go | 4 +- .../logictest/testdata/logic_test/namespace | 51 ++++++++++++++++--- pkg/sql/resolver.go | 51 +++++++++++-------- 3 files changed, 77 insertions(+), 29 deletions(-) diff --git a/pkg/ccl/partitionccl/zone_test.go b/pkg/ccl/partitionccl/zone_test.go index 61390c5f219a..37e5fbb6cff8 100644 --- a/pkg/ccl/partitionccl/zone_test.go +++ b/pkg/ccl/partitionccl/zone_test.go @@ -220,11 +220,11 @@ func TestInvalidIndexPartitionSetShowZones(t *testing.T) { }{ { "ALTER INDEX foo EXPERIMENTAL CONFIGURE ZONE ''", - `no database specified: "foo"`, + `no schema has been selected to search index: "foo"`, }, { "EXPERIMENTAL SHOW ZONE CONFIGURATION FOR INDEX foo", - `no database specified: "foo"`, + `no schema has been selected to search index: "foo"`, }, { "USE system; ALTER INDEX foo EXPERIMENTAL CONFIGURE ZONE ''", diff --git a/pkg/sql/logictest/testdata/logic_test/namespace b/pkg/sql/logictest/testdata/logic_test/namespace index 26e49854f3c3..9a236bd79f17 100644 --- a/pkg/sql/logictest/testdata/logic_test/namespace +++ b/pkg/sql/logictest/testdata/logic_test/namespace @@ -22,7 +22,7 @@ t # Of course one can also list the tables in "public" by making it the # current database. statement ok -SET DATABASE = public +SET database = public query T SHOW TABLES @@ -30,7 +30,7 @@ SHOW TABLES t statement ok -SET DATABASE = test; DROP DATABASE public +SET database = test; DROP DATABASE public # Unqualified pg_type resolves from pg_catalog. query T @@ -40,7 +40,7 @@ date # Override table and check name resolves properly. statement ok -SET SEARCH_PATH=public,pg_catalog +SET search_path=public,pg_catalog statement ok CREATE TABLE pg_type(x INT); INSERT INTO pg_type VALUES(42) @@ -53,23 +53,60 @@ SELECT x FROM pg_type # Leave database, check name resolves to default. # The expected error can only occur on the virtual pg_type, not the physical one. query error no database specified -SET DATABASE = ''; SELECT * FROM pg_type +SET database = ''; SELECT * FROM pg_type # Go to different database, check name still resolves to default. query T -CREATE DATABASE foo; SET DATABASE = foo; SELECT typname FROM pg_type WHERE typname = 'date' +CREATE DATABASE foo; SET database = foo; SELECT typname FROM pg_type WHERE typname = 'date' ---- date # Verify that pg_catalog at the beginning of the search path takes precedence. query T -SET DATABASE = test; SET SEARCH_PATH = pg_catalog,public; SELECT typname FROM pg_type WHERE typname = 'date' +SET database = test; SET search_path = pg_catalog,public; SELECT typname FROM pg_type WHERE typname = 'date' ---- date # Now set the search path to the testdb, placing pg_catalog explicitly # at the end. query I -SET SEARCH_PATH = public,pg_catalog; SELECT x FROM pg_type +SET search_path = public,pg_catalog; SELECT x FROM pg_type ---- 42 + +statement ok +DROP TABLE pg_type; RESET search_path; SET database = test + +# Unqualified index name resolution. +statement ok +ALTER INDEX "primary" RENAME TO a_pk + +# Schema-qualified index name resolution. +statement ok +ALTER INDEX public.a_pk RENAME TO a_pk2 + +# DB-qualified index name resolution (CRDB 1.x compat). +statement ok +ALTER INDEX test.a_pk2 RENAME TO a_pk3 + +statement ok +CREATE DATABASE public; CREATE TABLE public.public.t(a INT) + +# We can't see the DB "public" with DB-qualified index name resolution. +statement error index "primary" does not exist +ALTER INDEX public."primary" RENAME TO t_pk + +# But we can see it with sufficient qualification. +statement ok +ALTER INDEX public.public."primary" RENAME TO t_pk + +# If the search path is invalid, we get a special error. +statement ok +SET search_path = invalid + +statement error no schema has been selected to search index: "a_pk3" +ALTER INDEX a_pk3 RENAME TO a_pk4 + +# But qualification resolves the problem. +statement ok +ALTER INDEX public.a_pk3 RENAME TO a_pk4 diff --git a/pkg/sql/resolver.go b/pkg/sql/resolver.go index d6cc917d8c23..ef89fc18fe92 100644 --- a/pkg/sql/resolver.go +++ b/pkg/sql/resolver.go @@ -290,8 +290,8 @@ func (p *planner) getQualifiedTableName( return tbName.String(), nil } -// findTableContainingIndex returns the name of the table containing an -// index of the given name and its descriptor. +// findTableContainingIndex returns the descriptor of a table +// containing the index of the given name. // This is used by expandIndexName(). // // An error is returned if the index name is ambiguous (i.e. exists in @@ -299,14 +299,17 @@ func (p *planner) getQualifiedTableName( // error will be returned, otherwise the TableName and descriptor // returned will be nil. func findTableContainingIndex( - sc SchemaAccessor, dbName string, idxName tree.UnrestrictedName, lookupFlags CommonLookupFlags, + sc SchemaAccessor, + dbName, scName string, + idxName tree.UnrestrictedName, + lookupFlags CommonLookupFlags, ) (result *tree.TableName, desc *sqlbase.TableDescriptor, err error) { dbDesc, err := sc.GetDatabaseDesc(dbName, lookupFlags) if dbDesc == nil || err != nil { return nil, nil, err } - tns, err := sc.GetObjectNames(dbDesc, tree.PublicSchema, + tns, err := sc.GetObjectNames(dbDesc, scName, DatabaseListFlags{CommonLookupFlags: lookupFlags, explicitPrefix: true}) if err != nil { return nil, nil, err @@ -377,32 +380,40 @@ func expandIndexName( // subsequent call to expandIndexName() will generate tn using the // new value of index.Table, which is a table name. - // Just an assertion: if we got there, there cannot be a path prefix - // in tn or a value in index.Index yet. - if tn.ExplicitSchema || tn.ExplicitCatalog || index.Index != "" { + // Just an assertion: if we got there, there cannot be a value in index.Index yet. + if index.Index != "" { return nil, nil, pgerror.NewErrorf(pgerror.CodeInternalError, "programmer error: not-searched index name found already qualified: %s@%s", tn, index.Index) } - curDb := sc.CurrentDatabase() - if curDb == "" { - return nil, nil, pgerror.NewErrorf(pgerror.CodeUndefinedObjectError, - "no database specified: %q", tree.ErrString(index)) + index.Index = tree.UnrestrictedName(tn.TableName) + + // Look up the table prefix. + found, _, err := tn.TableNamePrefix.Resolve(ctx, sc, sc.CurrentDatabase(), sc.CurrentSearchPath()) + if err != nil { + return nil, nil, err + } + if !found { + if requireTable { + return nil, nil, pgerror.NewErrorf(pgerror.CodeUndefinedObjectError, + "no schema has been selected to search index: %q", + tree.ErrString(&index.Index)).SetHintf( + "check the current database and search_path are valid") + } + return nil, nil, nil } - index.Index = tree.UnrestrictedName(tn.TableName) lookupFlags := sc.CommonLookupFlags(ctx, requireTable) - tn, desc, err = findTableContainingIndex( - sc.LogicalSchemaAccessor(), curDb, index.Index, lookupFlags) + var foundTn *tree.TableName + foundTn, desc, err = findTableContainingIndex( + sc.LogicalSchemaAccessor(), tn.Catalog(), tn.Schema(), index.Index, lookupFlags) if err != nil { return nil, nil, err - } else if tn == nil { - // NB: tn is nil here if and only if requireTable is - // false, otherwise err would be non-nil. - return nil, nil, nil + } else if foundTn != nil { + // Memoize the table name that was found. tn is a reference to the table name + // stored in index.Table. + *tn = *foundTn } - // Memoize the resolved table name in case expandIndexName() is called again. - index.Table.TableNameReference = tn } return tn, desc, nil } From 2f12b6869118109486133e650cc5fcbe2dcdeda0 Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Sun, 15 Apr 2018 19:03:03 +0200 Subject: [PATCH 2/2] sql: use a reusable name resolution algo for star expansions A previous patch has added a reusable name resolution mechanism which encapsulates all the special cases supported CockroachDB into a single code path. This covers both the resolution of object names and column names. Prior to this patch however, the expansion of qualified stars was using a separate code path. This proved to be incorrect/insufficient however, because one of the special cases of name resolution should apply to star expansion too and the separate code path did not contain the special case. To solve this problem, this patch abstracts the resolution of qualified stars in a common algorithm alongside the others (in `sem/name_resolution.go`) and ensures it is used where applicable. Release note (bug fix): CockroachDB now again allows to use a simply qualified table name in qualified stars (e.g., `SELECT mydb.kv.* FROM kv`), for compatibility with CockroachDB v1.x. --- pkg/sql/data_source.go | 10 +--- pkg/sql/logictest/testdata/logic_test/insert | 2 +- pkg/sql/logictest/testdata/logic_test/select | 25 +++++++- pkg/sql/sem/tree/name_resolution.go | 30 ++++++++++ pkg/sql/sem/tree/name_resolution_test.go | 61 ++++++++++++++++++++ 5 files changed, 116 insertions(+), 12 deletions(-) diff --git a/pkg/sql/data_source.go b/pkg/sql/data_source.go index 33960870e89e..9c0fe6a24950 100644 --- a/pkg/sql/data_source.go +++ b/pkg/sql/data_source.go @@ -506,19 +506,11 @@ func expandStar( } } case *tree.AllColumnsSelector: - tn, err := tree.NormalizeTableName(&sel.TableName) - if err != nil { - return nil, nil, err - } resolver := sqlbase.ColumnResolver{Sources: src} - numRes, _, _, err := resolver.FindSourceMatchingName(ctx, tn) + _, _, err := sel.Resolve(ctx, &resolver) if err != nil { return nil, nil, err } - if numRes == tree.NoResults { - return nil, nil, pgerror.NewErrorf(pgerror.CodeUndefinedColumnError, - "no data source named %q", tree.ErrString(&tn)) - } ds := src[resolver.ResolverState.SrcIdx] colSet := ds.SourceAliases[resolver.ResolverState.ColSetIdx].ColumnSet for i, ok := colSet.Next(0); ok; i, ok = colSet.Next(i + 1) { diff --git a/pkg/sql/logictest/testdata/logic_test/insert b/pkg/sql/logictest/testdata/logic_test/insert index 15f6b9888a6f..95e318187c7b 100644 --- a/pkg/sql/logictest/testdata/logic_test/insert +++ b/pkg/sql/logictest/testdata/logic_test/insert @@ -511,7 +511,7 @@ INSERT INTO return AS r VALUES (5, 6) statement ok INSERT INTO return VALUES (5, 6) RETURNING test.return.a -statement error no data source named "x" +statement error no data source matches pattern: x.\* INSERT INTO return VALUES (1, 2) RETURNING x.*[1] statement error column name "x" not found diff --git a/pkg/sql/logictest/testdata/logic_test/select b/pkg/sql/logictest/testdata/logic_test/select index d44b49238400..fd8b44739aa5 100644 --- a/pkg/sql/logictest/testdata/logic_test/select +++ b/pkg/sql/logictest/testdata/logic_test/select @@ -139,7 +139,28 @@ SELECT kv.* FROM kv ---- a NULL -query error no data source named "foo" +# Regression tests for #24169 +query TT +SELECT test.kv.* FROM kv +---- +a NULL + +query TT +SELECT test.public.kv.* FROM kv +---- +a NULL + +query TT +SELECT test.public.kv.* FROM test.kv +---- +a NULL + +query TT +SELECT test.kv.* FROM test.public.kv +---- +a NULL + +query error no data source matches pattern: foo.\* SELECT foo.* FROM kv query error cannot use "\*" without a FROM clause @@ -156,7 +177,7 @@ SELECT 1, * FROM nocols query error "kv.*" cannot be aliased SELECT kv.* AS foo FROM kv -query error no data source named "bar.kv" +query error no data source matches pattern: bar.kv.\* SELECT bar.kv.* FROM kv # Don't panic with invalid names (#8024) diff --git a/pkg/sql/sem/tree/name_resolution.go b/pkg/sql/sem/tree/name_resolution.go index 3ef1397e4472..f3620769fac9 100644 --- a/pkg/sql/sem/tree/name_resolution.go +++ b/pkg/sql/sem/tree/name_resolution.go @@ -186,6 +186,36 @@ type ColumnResolutionResult interface { ColumnResolutionResult() } +// Resolve performs name resolution for a qualified star using a resolver. +func (a *AllColumnsSelector) Resolve( + ctx context.Context, r ColumnItemResolver, +) (srcName *TableName, srcMeta ColumnSourceMeta, err error) { + prefix := makeTableNameFromUnresolvedName(&a.TableName) + + // Is there a data source with this prefix? + var res NumResolutionResults + res, srcName, srcMeta, err = r.FindSourceMatchingName(ctx, prefix) + if err != nil { + return nil, nil, err + } + if res == NoResults && a.TableName.NumParts == 2 { + // No, but name of form db.tbl.*? + // Special rule for compatibility with CockroachDB v1.x: + // search name db.public.tbl.* instead. + prefix.ExplicitCatalog = true + prefix.CatalogName = prefix.SchemaName + prefix.SchemaName = PublicSchemaName + res, srcName, srcMeta, err = r.FindSourceMatchingName(ctx, prefix) + if err != nil { + return nil, nil, err + } + } + if res == NoResults { + return nil, nil, newSourceNotFoundError("no data source matches pattern: %s", a) + } + return srcName, srcMeta, nil +} + // Resolve performs name resolution for a column item using a resolver. func (c *ColumnItem) Resolve( ctx context.Context, r ColumnItemResolver, diff --git a/pkg/sql/sem/tree/name_resolution_test.go b/pkg/sql/sem/tree/name_resolution_test.go index 72525c3f91fa..f99cdc2967f3 100644 --- a/pkg/sql/sem/tree/name_resolution_test.go +++ b/pkg/sql/sem/tree/name_resolution_test.go @@ -390,6 +390,67 @@ func newFakeSource() *fakeSource { } } +func TestResolveQualifiedStar(t *testing.T) { + testCases := []struct { + in string + tnout string + csout string + err string + }{ + {`a.*`, ``, ``, `no data source matches pattern: a.*`}, + {`foo.*`, ``, ``, `ambiguous table name: foo`}, + {`db1.public.foo.*`, `db1.public.foo`, `x`, ``}, + {`db1.foo.*`, `db1.public.foo`, `x`, ``}, + {`dbx.foo.*`, ``, ``, `no data source matches pattern: dbx.foo.*`}, + {`kv.*`, `db1.public.kv`, `k, v`, ``}, + } + fakeFrom := newFakeSource() + for _, tc := range testCases { + t.Run(tc.in, func(t *testing.T) { + fakeFrom.t = t + tnout, csout, err := func() (string, string, error) { + stmt, err := parser.ParseOne(fmt.Sprintf("SELECT %s", tc.in)) + if err != nil { + return "", "", err + } + v := stmt.(*tree.Select).Select.(*tree.SelectClause).Exprs[0].Expr.(tree.VarName) + c, err := v.NormalizeVarName() + if err != nil { + return "", "", err + } + acs, ok := c.(*tree.AllColumnsSelector) + if !ok { + return "", "", fmt.Errorf("var name %s (%T) did not resolve to AllColumnsSelector, found %T instead", + v, v, c) + } + tn, res, err := acs.Resolve(context.Background(), fakeFrom) + if err != nil { + return "", "", err + } + cs, ok := res.(colsRes) + if !ok { + return "", "", fmt.Errorf("fake resolver did not return colsRes, found %T instead", res) + } + nl := tree.NameList(cs) + return tn.String(), nl.String(), nil + }() + if !testutils.IsError(err, tc.err) { + t.Fatalf("%s: expected %s, but found %v", tc.in, tc.err, err) + } + if tc.err != "" { + return + } + + if tc.tnout != tnout { + t.Fatalf("%s: expected tn %s, but found %s", tc.in, tc.tnout, tnout) + } + if tc.csout != csout { + t.Fatalf("%s: expected cs %s, but found %s", tc.in, tc.csout, csout) + } + }) + } +} + func TestResolveColumnItem(t *testing.T) { testCases := []struct { in string