Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

catalog: add implicit record types for all tables #70100

Merged
merged 3 commits into from
Oct 15, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions pkg/sql/alter_type.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,12 @@ func (p *planner) AlterType(ctx context.Context, n *tree.AlterType) (planNode, e
}
case descpb.TypeDescriptor_ENUM:
sqltelemetry.IncrementEnumCounter(sqltelemetry.EnumAlter)
case descpb.TypeDescriptor_TABLE_IMPLICIT_RECORD_TYPE:
return nil, pgerror.Newf(
pgcode.WrongObjectType,
"%q is a table's record type and cannot be modified",
tree.AsStringWithFQNames(n.Type, &p.semaCtx.Annotations),
)
}

return &alterTypeNode{
Expand Down
718 changes: 363 additions & 355 deletions pkg/sql/catalog/descpb/structured.pb.go

Large diffs are not rendered by default.

4 changes: 4 additions & 0 deletions pkg/sql/catalog/descpb/structured.proto
Original file line number Diff line number Diff line change
Expand Up @@ -1365,6 +1365,10 @@ message TypeDescriptor {
// Represents a special multi-region enum type which tracks available regions
// as its enum values.
MULTIREGION_ENUM = 2;
// Represents the implicit record type created on behalf of each table. This
// kind of TypeDescriptor is *never* persisted to disk! If you are here,
// thinking about using or persisting this value, you should *not* do that!
TABLE_IMPLICIT_RECORD_TYPE = 3;
// Add more entries as we support more user defined types.
}
optional Kind kind = 5 [(gogoproto.nullable) = false];
Expand Down
4 changes: 3 additions & 1 deletion pkg/sql/catalog/descriptor.go
Original file line number Diff line number Diff line change
Expand Up @@ -678,7 +678,9 @@ type TypeDescriptor interface {
// types.T. Implementers of tree.TypeReferenceResolver should implement this
// interface as well.
type TypeDescriptorResolver interface {
// GetTypeDescriptor returns the type descriptor for the input ID.
// GetTypeDescriptor returns the type descriptor for the input ID. Note that
// the returned type descriptor may be the implicitly-defined record type for
// a table, if the input ID points to a table descriptor.
GetTypeDescriptor(ctx context.Context, id descpb.ID) (tree.TypeName, TypeDescriptor, error)
}

Expand Down
3 changes: 1 addition & 2 deletions pkg/sql/catalog/descs/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,6 @@ go_test(
"//pkg/sql/catalog/descpb",
"//pkg/sql/catalog/lease",
"//pkg/sql/catalog/tabledesc",
"//pkg/sql/pgwire/pgcode",
"//pkg/sql/pgwire/pgerror",
"//pkg/sql/privilege",
"//pkg/sql/sem/tree",
"//pkg/sql/tests",
Expand All @@ -102,6 +100,7 @@ go_test(
"//pkg/util/leaktest",
"//pkg/util/log",
"//pkg/util/randutil",
"@com_github_lib_pq//oid",
"@com_github_stretchr_testify//require",
],
)
24 changes: 17 additions & 7 deletions pkg/sql/catalog/descs/collection_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,6 @@ import (
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descs"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/lease"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
"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/tests"
Expand All @@ -38,6 +36,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/testutils/testcluster"
"github.com/cockroachdb/cockroach/pkg/util/leaktest"
"github.com/cockroachdb/cockroach/pkg/util/log"
"github.com/lib/pq/oid"
"github.com/stretchr/testify/require"
)

Expand Down Expand Up @@ -422,7 +421,7 @@ func TestSyntheticDescriptorResolution(t *testing.T) {

// Regression test to ensure that resolving a type descriptor which is not a
// type using the DistSQLTypeResolver is properly handled.
func TestDistSQLTypeResolver_GetTypeDescriptor_WrongType(t *testing.T) {
func TestDistSQLTypeResolver_GetTypeDescriptor_FromTable(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)

Expand All @@ -433,20 +432,31 @@ func TestDistSQLTypeResolver_GetTypeDescriptor_WrongType(t *testing.T) {
s := tc.Server(0)

tdb := sqlutils.MakeSQLRunner(tc.ServerConn(0))
tdb.Exec(t, `CREATE TABLE t()`)
tdb.Exec(t, `CREATE TABLE t(a INT PRIMARY KEY, b STRING)`)
var id descpb.ID
tdb.QueryRow(t, "SELECT $1::regclass::int", "t").Scan(&id)

execCfg := s.ExecutorConfig().(sql.ExecutorConfig)
var name tree.TypeName
var typedesc catalog.TypeDescriptor
err := sql.DescsTxn(ctx, &execCfg, func(
ctx context.Context, txn *kv.Txn, descriptors *descs.Collection,
) error {
tr := descs.NewDistSQLTypeResolver(descriptors, txn)
_, _, err := tr.GetTypeDescriptor(ctx, id)
var err error
name, typedesc, err = tr.GetTypeDescriptor(ctx, id)
return err
})
require.Regexp(t, `descriptor \d+ is a relation not a type`, err)
require.Equal(t, pgcode.WrongObjectType, pgerror.GetPGCode(err))
require.NoError(t, err)
require.Equal(t, "t", name.ObjectName.String())
typ, err := typedesc.MakeTypesT(ctx, &name, nil)
require.NoError(t, err)
require.Equal(t, types.TupleFamily, typ.Family())
require.Equal(t, "t", typ.TypeMeta.Name.Name)
require.Equal(t, []string{"a", "b"}, typ.TupleLabels())
require.Equal(t, types.IntFamily, typ.TupleContents()[0].Family())
require.Equal(t, types.StringFamily, typ.TupleContents()[1].Family())
require.Equal(t, oid.Oid(id+100000), typ.Oid())
}

// TestMaybeFixSchemaPrivilegesIntegration ensures that schemas that have
Expand Down
21 changes: 19 additions & 2 deletions pkg/sql/catalog/descs/dist_sql_type_resolver.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,25 @@ func (dt DistSQLTypeResolver) GetTypeDescriptor(
if err != nil {
return tree.TypeName{}, nil, err
}
typeDesc, isType := desc.(catalog.TypeDescriptor)
if !isType {
var typeDesc catalog.TypeDescriptor
switch t := desc.(type) {
case catalog.TypeDescriptor:
// User-defined type.
typeDesc = t
case catalog.TableDescriptor:
// If we find a table descriptor when we were expecting a type descriptor,
// we return the implicitly-created type descriptor that is created for each
// table. Make sure that we hydrate the table ahead of time, since we expect
// that the table's types are fully hydrated below.
t, err = dt.descriptors.hydrateTypesInTableDesc(ctx, dt.txn, t)
if err != nil {
return tree.TypeName{}, nil, err
}
typeDesc, err = typedesc.CreateImplicitRecordTypeFromTableDesc(t)
if err != nil {
return tree.TypeName{}, nil, err
}
default:
return tree.TypeName{}, nil, pgerror.Newf(pgcode.WrongObjectType,
"descriptor %d is a %s not a %s", id, desc.DescriptorType(), catalog.Type)
}
Expand Down
30 changes: 28 additions & 2 deletions pkg/sql/catalog/descs/object.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ import (

"github.com/cockroachdb/cockroach/pkg/kv"
"github.com/cockroachdb/cockroach/pkg/sql/catalog"
"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/sem/tree"
"github.com/cockroachdb/cockroach/pkg/sql/sqlerrors"
"github.com/cockroachdb/errors"
Expand Down Expand Up @@ -86,13 +89,36 @@ func (tc *Collection) getObjectByName(
}
switch t := desc.(type) {
case catalog.TableDescriptor:
if flags.DesiredObjectKind != tree.TableObject {
// A given table name can resolve to either a type descriptor or a table
// descriptor, because every table descriptor also defines an implicit
// record type with the same name as the table. Thus, depending on the
// requested descriptor type, we return either the table descriptor itself,
// or the table descriptor's implicit record type.
switch flags.DesiredObjectKind {
case tree.TableObject, tree.TypeObject:
default:
return prefix, nil, nil
}
desc, err = tc.hydrateTypesInTableDesc(ctx, txn, t)
tableDesc, err := tc.hydrateTypesInTableDesc(ctx, txn, t)
if err != nil {
return prefix, nil, err
}
desc = tableDesc
if flags.DesiredObjectKind == tree.TypeObject {
// Since a type descriptor was requested, we need to return the implicitly
// created record type for the table that we found.
if flags.RequireMutable {
// ... but, we can't do it if we need a mutable descriptor - we don't
// have the capability of returning a mutable type descriptor for a
// table's implicit record type.
return prefix, nil, pgerror.Newf(pgcode.InsufficientPrivilege,
"cannot modify table record type %q", objectName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: you could construct a fully qualified name here rather cheaply and then let the tree type do the quoting as necessary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason not to do that is that the by-id path will return a different error. I'm sort of okay with that. Take it or leave it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a bunch of %q around here, I think we could change them all in a batch rather than do just this one.

}
desc, err = typedesc.CreateImplicitRecordTypeFromTableDesc(tableDesc)
if err != nil {
return prefix, nil, err
}
}
case catalog.TypeDescriptor:
if flags.DesiredObjectKind != tree.TypeObject {
return prefix, nil, nil
Expand Down
27 changes: 21 additions & 6 deletions pkg/sql/catalog/descs/type.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,14 @@ func (tc *Collection) GetMutableTypeByID(
if err != nil {
return nil, err
}
return desc.(*typedesc.Mutable), nil
switch t := desc.(type) {
case *typedesc.Mutable:
return t, nil
case *typedesc.TableImplicitRecordType:
return nil, pgerror.Newf(pgcode.DependentObjectsStillExist, "cannot modify table record type %q", desc.GetName())
}
return nil,
errors.AssertionFailedf("unhandled type descriptor type %T during GetMutableTypeByID", desc)
}

// GetImmutableTypeByID returns an immutable type descriptor with
Expand All @@ -112,10 +119,18 @@ func (tc *Collection) getTypeByID(
}
return nil, err
}
typ, ok := desc.(catalog.TypeDescriptor)
if !ok {
return nil, pgerror.Newf(
pgcode.UndefinedObject, "type with ID %d does not exist", typeID)
switch t := desc.(type) {
case catalog.TypeDescriptor:
// User-defined type.
return t, nil
case catalog.TableDescriptor:
// Table record type.
t, err = tc.hydrateTypesInTableDesc(ctx, txn, t)
if err != nil {
return nil, err
}
return typedesc.CreateImplicitRecordTypeFromTableDesc(t)
}
return typ, nil
return nil, pgerror.Newf(
pgcode.UndefinedObject, "type with ID %d does not exist", typeID)
}
14 changes: 10 additions & 4 deletions pkg/sql/catalog/resolver/resolver.go
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,16 @@ func ResolveMutableType(
if err != nil || desc == nil {
return catalog.ResolvedObjectPrefix{}, nil, err
}
return prefix, desc.(*typedesc.Mutable), nil
switch t := desc.(type) {
case *typedesc.Mutable:
return prefix, t, nil
case *typedesc.TableImplicitRecordType:
return catalog.ResolvedObjectPrefix{}, nil, pgerror.Newf(pgcode.DependentObjectsStillExist,
"cannot modify table record type %q", desc.GetName())
default:
return catalog.ResolvedObjectPrefix{}, nil,
errors.AssertionFailedf("unhandled type descriptor type %T during resolve mutable desc", t)
}
}

// ResolveExistingObject resolves an object with the given flags.
Expand Down Expand Up @@ -214,9 +223,6 @@ func ResolveExistingObject(
if !isType {
return nil, prefix, sqlerrors.NewUndefinedTypeError(&resolvedTn)
}
if lookupFlags.RequireMutable {
return obj.(*typedesc.Mutable), prefix, nil
}
return typ, prefix, nil
case tree.TableObject:
table, ok := obj.(catalog.TableDescriptor)
Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/catalog/table_elements.go
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ type Index interface {
GetCompositeColumnID(compositeColumnOrdinal int) descpb.ColumnID
}

// Column is an interface around the index descriptor types.
// Column is an interface around the column descriptor types.
type Column interface {
TableElementMaybeMutation

Expand Down
1 change: 1 addition & 0 deletions pkg/sql/catalog/typedesc/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ go_library(
name = "typedesc",
srcs = [
"safe_format.go",
"table_implicit_record_type.go",
"type_desc.go",
"type_desc_builder.go",
],
Expand Down
Loading