From 14d91fb5aaf7c64d7cf1ff7b7541bd06ce536ea3 Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Fri, 13 Apr 2018 10:58:57 +0200 Subject: [PATCH] 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 | 50 ++++++++++-------- 3 files changed, 76 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 45f3c2fe1011..5cc4eae632a4 100644 --- a/pkg/sql/resolver.go +++ b/pkg/sql/resolver.go @@ -283,8 +283,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 @@ -292,14 +292,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 @@ -370,32 +373,39 @@ 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 = *foundTn } - // Memoize the resolved table name in case expandIndexName() is called again. - index.Table.TableNameReference = tn } return tn, desc, nil }