Skip to content

Commit

Permalink
sql: fix name resolution to avoid empty-name db lookups
Browse files Browse the repository at this point in the history
The internal executor has a current DB of "", and it also executes
against queries against system tables using names like `system.users`.
The name resolution logic turned this into a lookup of "".system.users,
which was not desired.

Now we explicitly only allow the "" DB name if the schema name
corresponds to a virtual schema.

Release note: none
  • Loading branch information
rafiss committed Jan 11, 2021
1 parent b6075a2 commit 45fb9cf
Show file tree
Hide file tree
Showing 4 changed files with 25 additions and 12 deletions.
2 changes: 1 addition & 1 deletion pkg/bench/ddl_analysis/testdata/benchmark_expectations
Original file line number Diff line number Diff line change
Expand Up @@ -66,5 +66,5 @@ min max benchmark
8 8 VirtualTableQueries/select_crdb_internal.invalid_objects_with_1_fk
1 1 VirtualTableQueries/select_crdb_internal.tables_with_1_fk
3 3 SystemDatabaseQueries/select_system.users_without_schema_name
6 6 SystemDatabaseQueries/select_system.users_with_empty_database_name
2 2 SystemDatabaseQueries/select_system.users_with_empty_database_name
2 2 SystemDatabaseQueries/select_system.users_with_schema_name
25 changes: 14 additions & 11 deletions pkg/sql/sem/tree/name_resolution.go
Original file line number Diff line number Diff line change
Expand Up @@ -308,19 +308,22 @@ func ResolveExisting(
// Two parts: D.T.
// Try to use the current database, and be satisfied if it's sufficient to find the object.
//
// Note: we test this even if curDb == "", because CockroachDB
// supports querying virtual schemas even when the current
// database is not set. For example, `select * from
// pg_catalog.pg_tables` is meant to show all tables across all
// databases when there is no current database set.

if found, objMeta, err := r.LookupObject(ctx, lookupFlags, curDb, scName, u.Object()); found || err != nil {
if err == nil {
namePrefix.CatalogName = Name(curDb)
namePrefix.SchemaName = Name(scName)
// Note: CockroachDB supports querying virtual schemas even when the current
// database is not set. For example, `select * from pg_catalog.pg_tables` is
// meant to show all tables across all databases when there is no current
// database set. Therefore, we test this even if curDb == "", as long as the
// schema name is for a virtual schema.

if _, isVirtualSchema := sessiondata.VirtualSchemaNames[scName]; isVirtualSchema || curDb != "" {
if found, objMeta, err := r.LookupObject(ctx, lookupFlags, curDb, scName, u.Object()); found || err != nil {
if err == nil {
namePrefix.CatalogName = Name(curDb)
namePrefix.SchemaName = Name(scName)
}
return found, namePrefix, objMeta, err
}
return found, namePrefix, objMeta, err
}

// No luck so far. Compatibility with CockroachDB v1.1: try D.public.T instead.
if found, objMeta, err := r.LookupObject(ctx, lookupFlags, u.Schema(), PublicSchema, u.Object()); found || err != nil {
if err == nil {
Expand Down
2 changes: 2 additions & 0 deletions pkg/sql/sem/tree/name_resolution_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -529,6 +529,7 @@ func newFakeMetadata() *fakeMetadata {
{"public", []tree.Name{"foo", "bar"}},
{"pg_temp_123", []tree.Name{"foo", "baz"}},
}},
{"system", []knownSchema{{"public", []tree.Name{"users"}}}},
},
}
}
Expand Down Expand Up @@ -624,6 +625,7 @@ func TestResolveTablePatternOrName(t *testing.T) {
{`kv`, ``, mpath("public", "pg_catalog"), true, ``, ``, ``, `prefix or object not found`},
{`pg_tables`, ``, mpath("public", "pg_catalog"), true, `pg_tables`, `"".pg_catalog.pg_tables`, `.pg_catalog[0]`, ``},
{`pg_tables`, ``, mpath(), true, `pg_tables`, `"".pg_catalog.pg_tables`, `.pg_catalog[0]`, ``},
{`system.users`, ``, mpath(), true, `system.public.users`, `system.public.users`, `system.public[0]`, ``},

{`blix`, ``, mpath("public"), false, ``, ``, ``, `prefix or object not found`},
{`blix`, ``, mpath("public", "pg_catalog"), false, `blix`, `"".pg_catalog.blix`, `.pg_catalog`, ``},
Expand Down
8 changes: 8 additions & 0 deletions pkg/sql/sessiondata/search_path.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,14 @@ const PgTempSchemaName = "pg_temp"
// when installing an extension, but must be stored as a separate schema in CRDB.
const PgExtensionSchemaName = "pg_extension"

// VirtualSchemaNames is a set of all virtual schema names.
var VirtualSchemaNames = map[string]struct{}{
PgCatalogName: {},
InformationSchemaName: {},
CRDBInternalSchemaName: {},
PgExtensionSchemaName: {},
}

// DefaultSearchPath is the search path used by virgin sessions.
var DefaultSearchPath = MakeSearchPath(
[]string{UserSchemaName, PublicSchemaName},
Expand Down

0 comments on commit 45fb9cf

Please sign in to comment.