From 9db726a96385b9d9153c0eff2d46be7086d9e632 Mon Sep 17 00:00:00 2001 From: Rohan Yadav Date: Wed, 23 Dec 2020 13:38:06 -0800 Subject: [PATCH] sql: ensure event log entries for ALTER/DROP TYPE are fully qualified Fixes #57734. Release note (bug fix): Fully qualified names in the `TypeName` field of the event log for `ALTER TYPE` and `DROP TYPE` statements. --- pkg/sql/alter_type.go | 7 +-- pkg/sql/drop_type.go | 33 ++++++++--- .../logictest/testdata/logic_test/event_log | 55 +++++++++++++++++++ 3 files changed, 83 insertions(+), 12 deletions(-) diff --git a/pkg/sql/alter_type.go b/pkg/sql/alter_type.go index 95013102e6ab..105e4ba376b0 100644 --- a/pkg/sql/alter_type.go +++ b/pkg/sql/alter_type.go @@ -85,6 +85,7 @@ func (p *planner) AlterType(ctx context.Context, n *tree.AlterType) (planNode, e func (n *alterTypeNode) startExec(params runParams) error { telemetry.Inc(n.n.Cmd.TelemetryCounter()) + typeName := tree.AsStringWithFQNames(n.n.Type, params.p.Ann()) eventLogDone := false var err error switch t := n.n.Cmd.(type) { @@ -97,9 +98,7 @@ func (n *alterTypeNode) startExec(params runParams) error { return err } err = params.p.logEvent(params.ctx, n.desc.ID, &eventpb.RenameType{ - // TODO(knz): This name is insufficiently qualified. - // See: https://github.com/cockroachdb/cockroach/issues/57734 - TypeName: n.desc.Name, + TypeName: typeName, NewTypeName: string(t.NewName), }) eventLogDone = true @@ -130,7 +129,7 @@ func (n *alterTypeNode) startExec(params runParams) error { if err := params.p.logEvent(params.ctx, n.desc.ID, &eventpb.AlterType{ - TypeName: n.desc.Name, + TypeName: typeName, }); err != nil { return err } diff --git a/pkg/sql/drop_type.go b/pkg/sql/drop_type.go index 7ed751e69564..d04a9d6d970c 100644 --- a/pkg/sql/drop_type.go +++ b/pkg/sql/drop_type.go @@ -28,9 +28,14 @@ import ( "github.com/cockroachdb/errors" ) +type typeToDrop struct { + desc *typedesc.Mutable + fqName string +} + type dropTypeNode struct { n *tree.DropType - td map[descpb.ID]*typedesc.Mutable + td map[descpb.ID]typeToDrop } // Use to satisfy the linter. @@ -47,7 +52,7 @@ func (p *planner) DropType(ctx context.Context, n *tree.DropType) (planNode, err node := &dropTypeNode{ n: n, - td: make(map[descpb.ID]*typedesc.Mutable), + td: make(map[descpb.ID]typeToDrop), } if n.DropBehavior == tree.DropCascade { return nil, unimplemented.NewWithIssue(51480, "DROP TYPE CASCADE is not yet supported") @@ -102,8 +107,21 @@ func (p *planner) DropType(ctx context.Context, n *tree.DropType) (planNode, err } // Record these descriptors for deletion. - node.td[typeDesc.ID] = typeDesc - node.td[mutArrayDesc.ID] = mutArrayDesc + node.td[typeDesc.ID] = typeToDrop{ + desc: typeDesc, + fqName: tree.AsStringWithFQNames(name, p.Ann()), + } + arrayFQName, err := getTypeNameFromTypeDescriptor( + oneAtATimeSchemaResolver{ctx, p}, + mutArrayDesc, + ) + if err != nil { + return nil, err + } + node.td[mutArrayDesc.ID] = typeToDrop{ + desc: mutArrayDesc, + fqName: arrayFQName.FQString(), + } } return node, nil } @@ -138,14 +156,13 @@ func (p *planner) canDropTypeDesc( } func (n *dropTypeNode) startExec(params runParams) error { - for _, typ := range n.td { + for _, toDrop := range n.td { + typ, fqName := toDrop.desc, toDrop.fqName if err := params.p.dropTypeImpl(params.ctx, typ, tree.AsStringWithFQNames(n.n, params.Ann()), true /* queueJob */); err != nil { return err } // Log a Drop Type event. - // TODO(knz): This logging is imperfect, see this issue: - // https://github.com/cockroachdb/cockroach/issues/57734 - if err := params.p.logEvent(params.ctx, typ.ID, &eventpb.DropType{TypeName: typ.Name}); err != nil { + if err := params.p.logEvent(params.ctx, typ.ID, &eventpb.DropType{TypeName: fqName}); err != nil { return err } } diff --git a/pkg/sql/logictest/testdata/logic_test/event_log b/pkg/sql/logictest/testdata/logic_test/event_log index 8266aa098922..85a75a48233b 100644 --- a/pkg/sql/logictest/testdata/logic_test/event_log +++ b/pkg/sql/logictest/testdata/logic_test/event_log @@ -854,3 +854,58 @@ statement ok DROP DATABASE atest CASCADE; DROP USER v; DROP USER u + +# Regression for #57734. Ensure that type names are fully qualified in the +# event log. +subtest regression_57734 + +statement ok +CREATE TYPE eventlog AS ENUM ('event', 'log') + +query ITT +SELECT "reportingID", "eventType", info::JSONB - 'Timestamp' - 'DescriptorID' + FROM system.eventlog + WHERE "eventType" = 'create_type' AND info::JSONB->>'TypeName' LIKE '%eventlog' +ORDER BY "timestamp", info +---- +1 create_type {"EventType": "create_type", "Statement": "CREATE TYPE defaultdb.public.eventlog AS ENUM ('event', 'log')", "TypeName": "defaultdb.public.eventlog", "User": "root"} + +statement ok +ALTER TYPE eventlog ADD VALUE 'test' + +statement ok +ALTER TYPE eventlog RENAME VALUE 'test' TO 'testing' + +statement ok +CREATE SCHEMA testing + +statement ok +ALTER TYPE eventlog SET SCHEMA testing; +ALTER TYPE testing.eventlog SET SCHEMA public + +statement ok +ALTER TYPE eventlog RENAME TO eventlog_renamed + +query ITT +SELECT "reportingID", "eventType", info::JSONB - 'Timestamp' - 'DescriptorID' + FROM system.eventlog + WHERE ("eventType" = 'alter_type' OR "eventType" = 'rename_type') AND info::JSONB->>'TypeName' LIKE '%eventlog%' +ORDER BY "timestamp", info +---- +1 alter_type {"EventType": "alter_type", "Statement": "ALTER TYPE defaultdb.public.eventlog ADD VALUE 'test'", "TypeName": "defaultdb.public.eventlog", "User": "root"} +1 alter_type {"EventType": "alter_type", "Statement": "ALTER TYPE defaultdb.public.eventlog RENAME VALUE 'test' TO 'testing'", "TypeName": "defaultdb.public.eventlog", "User": "root"} +1 alter_type {"EventType": "alter_type", "Statement": "ALTER TYPE defaultdb.public.eventlog SET SCHEMA testing", "TypeName": "defaultdb.public.eventlog", "User": "root"} +1 alter_type {"EventType": "alter_type", "Statement": "ALTER TYPE defaultdb.testing.eventlog SET SCHEMA public", "TypeName": "defaultdb.testing.eventlog", "User": "root"} +1 rename_type {"EventType": "rename_type", "NewTypeName": "eventlog_renamed", "Statement": "ALTER TYPE defaultdb.public.eventlog RENAME TO eventlog_renamed", "TypeName": "defaultdb.public.eventlog", "User": "root"} + +statement ok +DROP TYPE eventlog_renamed + +query ITT +SELECT "reportingID", "eventType", info::JSONB - 'Timestamp' - 'DescriptorID' + FROM system.eventlog + WHERE "eventType" = 'drop_type' AND info::JSONB->>'TypeName' LIKE '%eventlog%' +ORDER BY "timestamp", info +---- +1 drop_type {"EventType": "drop_type", "Statement": "DROP TYPE defaultdb.public.eventlog_renamed", "TypeName": "defaultdb.public.eventlog_renamed", "User": "root"} +1 drop_type {"EventType": "drop_type", "Statement": "DROP TYPE defaultdb.public.eventlog_renamed", "TypeName": "defaultdb.public._eventlog_renamed", "User": "root"}