Merge cockroachdb#58737 cockroachdb#59440

58737: sql: add event logs for SET SCHEMA statements r=the-ericwang35 a=the-ericwang35 Fixes cockroachdb#57741. Previously, we had event log entries for table/views/sequence renames, however no event log entry is generated for SET SCHEMA. This is inconsistent, as SET SCHEMA changes the way that objects are named. To address this, this patch adds event logs for SET SCHEMA statements. Release note (bug fix): add event logs for SET SCHEMA statements 59440: authors: add Yufen Huang to authors r=otan a=Elliebababa Release note: None Co-authored-by: Eric Wang <[email protected]> Co-authored-by: elliebababa <[email protected]>
adityamaru · Jan 26, 2021 · 04f694a · 04f694a
3 parents c195665 + 6ad34f3 + e9b0721
commit 04f694a
Show file tree

Hide file tree

Showing 18 changed files with 781 additions and 267 deletions.
diff --git a/AUTHORS b/AUTHORS
@@ -342,6 +342,7 @@ Yang Yuting <[email protected]>
 yangliang9004 <[email protected]>
 Yevgeniy Miretskiy <[email protected]> <[email protected]>
 Yosi Attias <[email protected]>
+Yufen Huang <[email protected]> <[email protected]>
 yuhit <[email protected]>
 Yulei Xiao <[email protected]>
 YZ Chin <[email protected]>

diff --git a/docs/generated/eventlog.md b/docs/generated/eventlog.md
@@ -1030,6 +1030,30 @@ encounters a problem and is reversed.
 | `DescriptorID` | The primary object descriptor affected by the operation. Set to zero for operations that don't affect descriptors. | no |
 | `MutationID` | The descriptor mutation that this schema change was processing. | no |
 
+### `set_schema`
+
+An event of type `set_schema` is recorded when a table, view, sequence or type's schema is changed.
+
+
+| Field | Description | Sensitive |
+|--|--|--|
+| `DescriptorName` | The old name of the affected descriptor. | yes |
+| `NewDescriptorName` | The new name of the affected descriptor. | yes |
+| `DescriptorType` | The descriptor type being changed (table, view, sequence, type). | yes |
+
+
+#### Common fields
+
+| Field | Description | Sensitive |
+|--|--|--|
+| `Timestamp` | The timestamp of the event. Expressed as nanoseconds since the Unix epoch. | no |
+| `EventType` | The type of the event. | no |
+| `Statement` | A normalized copy of the SQL statement that triggered the event. | yes |
+| `User` | The user account that triggered the event. | yes |
+| `DescriptorID` | The primary object descriptor affected by the operation. Set to zero for operations that don't affect descriptors. | no |
+| `ApplicationName` | The application name for the session where the event was emitted. This is included in the event to ease filtering of logging output by application. | yes |
+| `PlaceholderValues` | The mapping of SQL placeholders to their values, for prepared statements. | yes |
+
 ### `truncate_table`
 
 An event of type `truncate_table` is recorded when a table is truncated.

diff --git a/pkg/sql/alter_table.go b/pkg/sql/alter_table.go
@@ -444,7 +444,7 @@ func (n *alterTableNode) startExec(params runParams) error {
 				}
 
 				droppedViews = append(droppedViews, cascadedViews...)
-				droppedViews = append(droppedViews, qualifiedView.String())
+				droppedViews = append(droppedViews, qualifiedView.FQString())
 			}
 
 			// We cannot remove this column if there are computed columns that use it.

diff --git a/pkg/sql/alter_table_owner.go b/pkg/sql/alter_table_owner.go
@@ -121,7 +121,7 @@ func (p *planner) checkCanAlterTableAndSetNewOwner(
 	return p.logEvent(ctx,
 		desc.ID,
 		&eventpb.AlterTableOwner{
-			TableName: tn.String(),
+			TableName: tn.FQString(),
 			Owner:     newOwner.Normalized(),
 		})
 }

diff --git a/pkg/sql/alter_table_set_schema.go b/pkg/sql/alter_table_set_schema.go
@@ -21,6 +21,7 @@ import (
 	"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/util/log/eventpb"
 )
 
 type alterTableSetSchemaNode struct {
@@ -100,6 +101,12 @@ func (n *alterTableSetSchemaNode) startExec(params runParams) error {
 	schemaID := tableDesc.GetParentSchemaID()
 	databaseID := tableDesc.GetParentID()
 
+	kind := tree.GetTableType(tableDesc.IsSequence(), tableDesc.IsView(), tableDesc.GetIsMaterializedView())
+	oldName, err := p.getQualifiedTableName(ctx, tableDesc)
+	if err != nil {
+		return err
+	}
+
 	desiredSchemaID, err := p.prepareSetSchema(ctx, tableDesc, n.newSchema)
 	if err != nil {
 		return err
@@ -140,7 +147,25 @@ func (n *alterTableSetSchemaNode) startExec(params runParams) error {
 	newTbKey := catalogkv.MakeObjectNameKey(ctx, p.ExecCfg().Settings,
 		databaseID, desiredSchemaID, tableDesc.Name)
 
-	return p.writeNameKey(ctx, newTbKey, tableDesc.ID)
+	if err := p.writeNameKey(ctx, newTbKey, tableDesc.ID); err != nil {
+		return err
+	}
+
+	newName, err := p.getQualifiedTableName(ctx, tableDesc)
+	if err != nil {
+		return err
+	}
+
+	return p.logEvent(ctx,
+		desiredSchemaID,
+		&eventpb.SetSchema{
+			CommonEventDetails:    eventpb.CommonEventDetails{},
+			CommonSQLEventDetails: eventpb.CommonSQLEventDetails{},
+			DescriptorName:        oldName.FQString(),
+			NewDescriptorName:     newName.FQString(),
+			DescriptorType:        kind,
+		},
+	)
 }
 
 // ReadingOwnWrites implements the planNodeReadingOwnWrites interface.

diff --git a/pkg/sql/alter_type.go b/pkg/sql/alter_type.go
@@ -297,6 +297,11 @@ func (p *planner) setTypeSchema(ctx context.Context, n *alterTypeNode, schema st
 	typeDesc := n.desc
 	schemaID := typeDesc.GetParentSchemaID()
 
+	oldName, err := p.getQualifiedTypeName(ctx, typeDesc)
+	if err != nil {
+		return err
+	}
+
 	desiredSchemaID, err := p.prepareSetSchema(ctx, typeDesc, schema)
 	if err != nil {
 		return err
@@ -321,8 +326,26 @@ func (p *planner) setTypeSchema(ctx context.Context, n *alterTypeNode, schema st
 		return err
 	}
 
-	return p.performRenameTypeDesc(
+	if err := p.performRenameTypeDesc(
 		ctx, arrayDesc, arrayDesc.Name, desiredSchemaID, tree.AsStringWithFQNames(n.n, p.Ann()),
+	); err != nil {
+		return err
+	}
+
+	newName, err := p.getQualifiedTypeName(ctx, typeDesc)
+	if err != nil {
+		return err
+	}
+
+	return p.logEvent(ctx,
+		desiredSchemaID,
+		&eventpb.SetSchema{
+			CommonEventDetails:    eventpb.CommonEventDetails{},
+			CommonSQLEventDetails: eventpb.CommonSQLEventDetails{},
+			DescriptorName:        oldName.FQString(),
+			NewDescriptorName:     newName.FQString(),
+			DescriptorType:        "type",
+		},
 	)
 }
 

diff --git a/pkg/sql/comment_on_column.go b/pkg/sql/comment_on_column.go
@@ -102,7 +102,7 @@ func (n *commentOnColumnNode) startExec(params runParams) error {
 	return params.p.logEvent(params.ctx,
 		n.tableDesc.ID,
 		&eventpb.CommentOnColumn{
-			TableName:   tn.String(),
+			TableName:   tn.FQString(),
 			ColumnName:  string(n.n.ColumnItem.ColumnName),
 			Comment:     comment,
 			NullComment: n.n.Comment == nil,

diff --git a/pkg/sql/comment_on_index.go b/pkg/sql/comment_on_index.go
@@ -78,7 +78,7 @@ func (n *commentOnIndexNode) startExec(params runParams) error {
 	return params.p.logEvent(params.ctx,
 		n.tableDesc.ID,
 		&eventpb.CommentOnIndex{
-			TableName:   tn.String(),
+			TableName:   tn.FQString(),
 			IndexName:   string(n.n.Index.Index),
 			Comment:     comment,
 			NullComment: n.n.Comment == nil,

diff --git a/pkg/sql/drop_index.go b/pkg/sql/drop_index.go
@@ -430,7 +430,7 @@ func (p *planner) dropIndexByName(
 				return err
 			}
 
-			droppedViews = append(droppedViews, qualifiedView.String())
+			droppedViews = append(droppedViews, qualifiedView.FQString())
 			droppedViews = append(droppedViews, cascadedViews...)
 		}
 	}

diff --git a/pkg/sql/drop_table.go b/pkg/sql/drop_table.go
@@ -353,7 +353,7 @@ func (p *planner) dropTableImpl(
 		}
 
 		droppedViews = append(droppedViews, cascadedViews...)
-		droppedViews = append(droppedViews, qualifiedView.String())
+		droppedViews = append(droppedViews, qualifiedView.FQString())
 	}
 
 	err := p.removeTableComments(ctx, tableDesc)

diff --git a/pkg/sql/drop_view.go b/pkg/sql/drop_view.go
@@ -234,7 +234,7 @@ func (p *planner) dropViewImpl(
 				return cascadeDroppedViews, err
 			}
 			cascadeDroppedViews = append(cascadeDroppedViews, cascadedViews...)
-			cascadeDroppedViews = append(cascadeDroppedViews, qualifiedView.String())
+			cascadeDroppedViews = append(cascadeDroppedViews, qualifiedView.FQString())
 		}
 	}
 

diff --git a/pkg/sql/logictest/testdata/logic_test/event_log b/pkg/sql/logictest/testdata/logic_test/event_log
@@ -951,6 +951,40 @@ WHERE "eventType" = 'comment_on_table'
 ----
 1  {"Comment": "This is a table.", "EventType": "comment_on_table", "Statement": "COMMENT ON TABLE defaultdb.public.a IS 'This is a table.'", "TableName": "defaultdb.public.a", "User": "root"}
 
+# Test the event logs generated by commands that set schemas.
+subtest set_schema
+
+statement ok
+ALTER TABLE a SET SCHEMA testing
+
+statement ok
+CREATE SEQUENCE s
+
+statement ok
+CREATE SCHEMA test_sc
+
+statement ok
+ALTER SEQUENCE s SET SCHEMA testing
+
+statement ok
+CREATE VIEW v AS SELECT 1
+
+statement ok
+ALTER VIEW v SET SCHEMA test_sc
+
+query IT
+SELECT "reportingID", info::JSONB - 'Timestamp' - 'DescriptorID'
+FROM system.eventlog
+WHERE "eventType" = 'set_schema'
+ORDER BY "timestamp", info
+----
+1  {"DescriptorName": "defaultdb.public.eventlog", "DescriptorType": "type", "EventType": "set_schema", "NewDescriptorName": "defaultdb.testing.eventlog", "Statement": "ALTER TYPE defaultdb.public.eventlog SET SCHEMA testing", "User": "root"}
+1  {"DescriptorName": "defaultdb.testing.eventlog", "DescriptorType": "type", "EventType": "set_schema", "NewDescriptorName": "defaultdb.public.eventlog", "Statement": "ALTER TYPE defaultdb.testing.eventlog SET SCHEMA public", "User": "root"}
+1  {"DescriptorName": "defaultdb.public.a", "DescriptorType": "table", "EventType": "set_schema", "NewDescriptorName": "defaultdb.testing.a", "Statement": "ALTER TABLE a SET SCHEMA testing", "User": "root"}
+1  {"DescriptorName": "defaultdb.public.s", "DescriptorType": "sequence", "EventType": "set_schema", "NewDescriptorName": "defaultdb.testing.s", "Statement": "ALTER SEQUENCE s SET SCHEMA testing", "User": "root"}
+1  {"DescriptorName": "defaultdb.public.v", "DescriptorType": "view", "EventType": "set_schema", "NewDescriptorName": "defaultdb.test_sc.v", "Statement": "ALTER VIEW v SET SCHEMA test_sc", "User": "root"}
+
+
 # Test the event logs generated by commands that drop views.
 subtest eventlog_dropped_views
 

diff --git a/pkg/sql/reparent_database.go b/pkg/sql/reparent_database.go
@@ -191,18 +191,18 @@ func (n *reparentDatabaseNode) startExec(params runParams) error {
 				for _, ref := range tbl.GetDependedOnBy() {
 					dep, err := p.Descriptors().GetMutableTableVersionByID(ctx, ref.ID, p.txn)
 					if err != nil {
-						return errors.Wrapf(err, errStr, n.db.Name, tblName.String())
+						return errors.Wrapf(err, errStr, n.db.Name, tblName.FQString())
 					}
 					fqName, err := p.getQualifiedTableName(ctx, dep)
 					if err != nil {
 						return errors.Wrapf(err, errStr, n.db.Name, dep.Name)
 					}
-					names = append(names, fqName.String())
+					names = append(names, fqName.FQString())
 				}
 				return sqlerrors.NewDependentObjectErrorf(
 					"could not convert database %q into schema because %q has dependent objects %v",
 					n.db.Name,
-					tblName.String(),
+					tblName.FQString(),
 					names,
 				)
 			}

diff --git a/pkg/sql/resolver.go b/pkg/sql/resolver.go
@@ -448,6 +448,33 @@ func (p *planner) getQualifiedSchemaName(
 	}, nil
 }
 
+// getQualifiedTypeName returns the database-qualified name of the type
+// represented by the provided descriptor.
+func (p *planner) getQualifiedTypeName(
+	ctx context.Context, desc catalog.TypeDescriptor,
+) (*tree.TypeName, error) {
+	dbDesc, err := p.Descriptors().GetImmutableDatabaseByID(ctx, p.txn, desc.GetParentID(), tree.DatabaseLookupFlags{
+		Required: true,
+	})
+	if err != nil {
+		return nil, err
+	}
+
+	schemaID := desc.GetParentSchemaID()
+	resolvedSchema, err := p.Descriptors().GetImmutableSchemaByID(ctx, p.txn, schemaID, tree.SchemaLookupFlags{})
+	if err != nil {
+		return nil, err
+	}
+
+	typeName := tree.MakeNewQualifiedTypeName(
+		dbDesc.GetName(),
+		resolvedSchema.Name,
+		desc.GetName(),
+	)
+
+	return &typeName, nil
+}
+
 // findTableContainingIndex returns the descriptor of a table
 // containing the index of the given name.
 // This is used by expandMutableIndexName().