Skip to content

Commit

Permalink
Merge #62216
Browse files Browse the repository at this point in the history 
62216: sql: remove three KV lookups from pg_catalog queries r=ajwerner,arulajmani a=rafiss

Use the descriptors that have already been fetched, instead of
re-getting them.

Release note: None

Co-authored-by: Rafi Shamim <[email protected]>
  • Loading branch information
@rafiss
craig[bot] and rafiss committed Mar 26, 2021
2 parents 1c9289e + 6637b22 commit 8b137b4
Show file tree
Hide file tree
Showing 4 changed files with 72 additions and 68 deletions.
24 changes: 24 additions & 0 deletions pkg/bench/ddl_analysis/orm_queries_bench_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,30 @@ WHERE
OR t.typinput = 'array_in(cstring,oid,integer)'::REGPROCEDURE
OR t.typelem != 0`,
},

{
name: "pg_type",
setup: `CREATE TABLE t1(a int primary key, b int);`,
stmt: `SELECT * FROM pg_type`,
},

{
name: "pg_class",
setup: `CREATE TABLE t1(a int primary key, b int);`,
stmt: `SELECT * FROM pg_class`,
},

{
name: "pg_namespace",
setup: `CREATE TABLE t1(a int primary key, b int);`,
stmt: `SELECT * FROM pg_namespace`,
},

{
name: "pg_attribute",
setup: `CREATE TABLE t1(a int primary key, b int);`,
stmt: `SELECT * FROM pg_attribute`,
},
}

RunRoundTripBenchmark(b, tests)
Expand Down
12 changes: 8 additions & 4 deletions pkg/bench/ddl_analysis/testdata/benchmark_expectations
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,9 @@ exp,benchmark
32,DropDatabase/drop_database_1_table
41,DropDatabase/drop_database_2_tables
50,DropDatabase/drop_database_3_tables
19,DropRole/drop_1_role
29,DropRole/drop_2_roles
39,DropRole/drop_3_roles
15,DropRole/drop_1_role
25,DropRole/drop_2_roles
35,DropRole/drop_3_roles
23,DropSequence/drop_1_sequence
37,DropSequence/drop_2_sequences
51,DropSequence/drop_3_sequences
Expand All @@ -52,10 +52,14 @@ exp,benchmark
42,Grant/grant_all_on_3_tables
21,GrantRole/grant_1_role
24,GrantRole/grant_2_roles
5,ORMQueries/activerecord_type_introspection_query
2,ORMQueries/activerecord_type_introspection_query
2,ORMQueries/django_table_introspection_1_table
2,ORMQueries/django_table_introspection_4_tables
2,ORMQueries/django_table_introspection_8_tables
2,ORMQueries/pg_attribute
2,ORMQueries/pg_class
2,ORMQueries/pg_namespace
2,ORMQueries/pg_type
24,Revoke/revoke_all_on_1_table
33,Revoke/revoke_all_on_2_tables
42,Revoke/revoke_all_on_3_tables
Expand Down
91 changes: 36 additions & 55 deletions pkg/sql/information_schema.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,6 @@ import (
"github.com/cockroachdb/cockroach/pkg/sql/catalog/schemaexpr"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc"
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
"github.com/cockroachdb/cockroach/pkg/sql/privilege"
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
Expand Down Expand Up @@ -1799,7 +1797,7 @@ func forEachSchema(
}
for i := range userDefinedSchemas {
desc := userDefinedSchemas[i]
canSeeDescriptor, err := userCanSeeDescriptor(ctx, p, desc, false /* allowAdding */)
canSeeDescriptor, err := userCanSeeDescriptor(ctx, p, desc, db, false /* allowAdding */)
if err != nil {
return err
}
Expand Down Expand Up @@ -1853,27 +1851,20 @@ func forEachDatabaseDesc(
}
dbDescs = allDbDescs
} else {
// We can't just use dbContext here because we need to fetch the descriptor
// with privileges from kv.
fetchedDbDesc, err := catalogkv.GetDatabaseDescriptorsFromIDs(
ctx, p.txn, p.ExecCfg().Codec, []descpb.ID{dbContext.GetID()},
)
if err != nil {
if errors.Is(err, catalog.ErrDescriptorNotFound) {
return pgerror.Newf(pgcode.UndefinedDatabase, "database %s does not exist", dbContext.GetName())
}
return err
}
dbDescs = fetchedDbDesc
dbDescs = append(dbDescs, dbContext)
}

// Ignore databases that the user cannot see.
for _, dbDesc := range dbDescs {
canSeeDescriptor, err := userCanSeeDescriptor(ctx, p, dbDesc, false /* allowAdding */)
if err != nil {
return err
canSeeDescriptor := !requiresPrivileges
if requiresPrivileges {
var err error
canSeeDescriptor, err = userCanSeeDescriptor(ctx, p, dbDesc, nil /* parentDBDesc */, false /* allowAdding */)
if err != nil {
return err
}
}
if !requiresPrivileges || canSeeDescriptor {
if canSeeDescriptor {
if err := fn(dbDesc); err != nil {
return err
}
Expand All @@ -1896,23 +1887,19 @@ func forEachTypeDesc(
if err != nil {
return err
}
schemaNames, err := getSchemaNames(ctx, p, dbContext)
if err != nil {
return err
}
lCtx := newInternalLookupCtx(ctx, descs, dbContext,
catalogkv.NewOneLevelUncachedDescGetter(p.txn, p.execCfg.Codec))
for _, id := range lCtx.typIDs {
typ := lCtx.typDescs[id]
dbDesc, parentExists := lCtx.dbDescs[typ.ParentID]
if !parentExists {
dbDesc, err := lCtx.getDatabaseByID(typ.GetParentID())
if err != nil {
continue
}
scName, ok := schemaNames[typ.GetParentSchemaID()]
if !ok {
return errors.AssertionFailedf("schema id %d not found", typ.GetParentSchemaID())
scName, err := lCtx.getSchemaNameByID(typ.GetParentSchemaID())
if err != nil {
return err
}
canSeeDescriptor, err := userCanSeeDescriptor(ctx, p, typ, false /* allowAdding */)
canSeeDescriptor, err := userCanSeeDescriptor(ctx, p, typ, dbDesc, false /* allowAdding */)
if err != nil {
return err
}
Expand Down Expand Up @@ -2084,26 +2071,26 @@ func forEachTypeDescWithTableLookupInternalFromDescriptors(

for _, typID := range lCtx.typIDs {
typDesc := lCtx.typDescs[typID]
canSeeDescriptor, err := userCanSeeDescriptor(ctx, p, typDesc, allowAdding)
if typDesc.Dropped() {
continue
}
dbDesc, err := lCtx.getDatabaseByID(typDesc.GetParentID())
if err != nil {
return err
}
if typDesc.Dropped() || !canSeeDescriptor {
canSeeDescriptor, err := userCanSeeDescriptor(ctx, p, typDesc, dbDesc, allowAdding)
if err != nil {
return err
}
if !canSeeDescriptor {
continue
}
var scName string
dbDesc, parentExists := lCtx.dbDescs[typDesc.GetParentID()]
if parentExists {
var ok bool
scName, ok = lCtx.schemaNames[typDesc.GetParentSchemaID()]
if !ok {
return errors.AssertionFailedf("schema id %d not found", typDesc.GetParentSchemaID())
}
if err := fn(dbDesc, scName, typDesc, lCtx); err != nil {
return err
}
} else {
return errors.AssertionFailedf("database id %d not found", typDesc.GetParentID())
scName, err := lCtx.getSchemaNameByID(typDesc.GetParentSchemaID())
if err != nil {
return err
}
if err := fn(dbDesc, scName, typDesc, lCtx); err != nil {
return err
}
}
return nil
Expand Down Expand Up @@ -2157,15 +2144,15 @@ func forEachTableDescWithTableLookupInternalFromDescriptors(
// Physical descriptors next.
for _, tbID := range lCtx.tbIDs {
table := lCtx.tbDescs[tbID]
canSeeDescriptor, err := userCanSeeDescriptor(ctx, p, table, allowAdding)
dbDesc, parentExists := lCtx.dbDescs[table.GetParentID()]
canSeeDescriptor, err := userCanSeeDescriptor(ctx, p, table, dbDesc, allowAdding)
if err != nil {
return err
}
if table.Dropped() || !canSeeDescriptor {
continue
}
var scName string
dbDesc, parentExists := lCtx.dbDescs[table.GetParentID()]
if parentExists {
var ok bool
scName, ok = lCtx.schemaNames[table.GetParentSchemaID()]
Expand Down Expand Up @@ -2296,25 +2283,19 @@ func forEachRoleMembership(
}

func userCanSeeDescriptor(
ctx context.Context, p *planner, desc catalog.Descriptor, allowAdding bool,
ctx context.Context, p *planner, desc, parentDBDesc catalog.Descriptor, allowAdding bool,
) (bool, error) {
if !descriptorIsVisible(desc, allowAdding) {
return false, nil
}

found, dbDesc, err := p.Descriptors().GetImmutableDatabaseByID(
ctx, p.Txn(), desc.GetParentID(), tree.DatabaseLookupFlags{Required: false},
)
if err != nil {
return false, err
}
// TODO(richardjcai): We may possibly want to remove the ability to view
// the descriptor if they have any privilege on the descriptor and only
// allow the descriptor to be viewed if they have CONNECT on the DB. #59827.
canSeeDescriptor := p.CheckAnyPrivilege(ctx, desc) == nil
// Users can see objects in the database if they have connect privilege.
if found {
canSeeDescriptor = canSeeDescriptor || p.CheckPrivilege(ctx, dbDesc, privilege.CONNECT) == nil
if parentDBDesc != nil {
canSeeDescriptor = canSeeDescriptor || p.CheckPrivilege(ctx, parentDBDesc, privilege.CONNECT) == nil
}
return canSeeDescriptor, nil
}
Expand Down
13 changes: 4 additions & 9 deletions pkg/sql/pg_catalog.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1069,7 +1069,7 @@ func makeAllRelationsVirtualTableWithDescriptorIDIndex(
}
// Don't include tables that aren't in the current database unless
// they're virtual, dropped tables, or ones that the user can't see.
canSeeDescriptor, err := userCanSeeDescriptor(ctx, p, table, true /*allowAdding*/)
canSeeDescriptor, err := userCanSeeDescriptor(ctx, p, table, db, true /*allowAdding*/)
if err != nil {
return false, err
}
Expand Down Expand Up @@ -2480,14 +2480,9 @@ https://www.postgresql.org/docs/9.5/catalog-pg-type.html`,
}

// Now generate rows for user defined types in this database.
return forEachTypeDesc(ctx, p, dbContext, func(_ *dbdesc.Immutable, _ string, typDesc *typedesc.Immutable) error {
sc, err := p.Descriptors().GetImmutableSchemaByID(
ctx, p.txn, typDesc.ParentSchemaID, tree.SchemaLookupFlags{})
if err != nil {
return err
}
nspOid := h.NamespaceOid(db.GetID(), sc.Name)
typ, err := typDesc.MakeTypesT(ctx, tree.NewUnqualifiedTypeName(tree.Name(typDesc.GetName())), p)
return forEachTypeDesc(ctx, p, db, func(_ *dbdesc.Immutable, scName string, typDesc *typedesc.Immutable) error {
nspOid := h.NamespaceOid(db.GetID(), scName)
typ, err := typDesc.MakeTypesT(ctx, tree.NewQualifiedTypeName(db.Name, scName, typDesc.GetName()), p)
if err != nil {
return err
}
Expand Down

0 comments on commit 8b137b4

Please sign in to comment.