Skip to content

Commit

Permalink
sql: properly qualify the names of dropped views
Browse files Browse the repository at this point in the history
Fixes #57735.

Previously, event logs were not capturing the
fully qualified names of dropped views.
This PR changes the event logs to use the fully
qualified view names.
Tests were also updated to reflect this change.

Release note (bug fix): add qualification prefix for dropped views.
  • Loading branch information
the-ericwang35 committed Jan 24, 2021
1 parent 6bbf3ed commit 410188f
Show file tree
Hide file tree
Showing 7 changed files with 110 additions and 19 deletions.
15 changes: 10 additions & 5 deletions pkg/sql/alter_table.go
Original file line number Diff line number Diff line change
Expand Up @@ -437,10 +437,17 @@ func (n *alterTableNode) startExec(params runParams) error {
}
jobDesc := fmt.Sprintf("removing view %q dependent on column %q which is being dropped",
viewDesc.Name, colToDrop.ColName())
droppedViews, err = params.p.removeDependentView(params.ctx, n.tableDesc, viewDesc, jobDesc)
cascadedViews, err := params.p.removeDependentView(params.ctx, n.tableDesc, viewDesc, jobDesc)
if err != nil {
return err
}
qualifiedView, err := params.p.getQualifiedTableName(params.ctx, viewDesc)
if err != nil {
return err
}

droppedViews = append(droppedViews, cascadedViews...)
droppedViews = append(droppedViews, qualifiedView.String())
}

// We cannot remove this column if there are computed columns that use it.
Expand Down Expand Up @@ -924,10 +931,8 @@ func (n *alterTableNode) startExec(params runParams) error {
return params.p.logEvent(params.ctx,
n.tableDesc.ID,
&eventpb.AlterTable{
TableName: params.p.ResolvedName(n.n.Table).FQString(),
MutationID: uint32(mutationID),
// TODO(knz): This is missing some qualification, see
// https://github.com/cockroachdb/cockroach/issues/57735
TableName: params.p.ResolvedName(n.n.Table).FQString(),
MutationID: uint32(mutationID),
CascadeDroppedViews: droppedViews,
})
}
Expand Down
16 changes: 10 additions & 6 deletions pkg/sql/drop_index.go
Original file line number Diff line number Diff line change
Expand Up @@ -424,7 +424,13 @@ func (p *planner) dropIndexByName(
if err != nil {
return err
}
droppedViews = append(droppedViews, viewDesc.Name)

qualifiedView, err := p.getQualifiedTableName(ctx, viewDesc)
if err != nil {
return err
}

droppedViews = append(droppedViews, qualifiedView.String())
droppedViews = append(droppedViews, cascadedViews...)
}
}
Expand Down Expand Up @@ -522,11 +528,9 @@ func (p *planner) dropIndexByName(
return p.logEvent(ctx,
tableDesc.ID,
&eventpb.DropIndex{
TableName: tn.FQString(),
IndexName: string(idxName),
MutationID: uint32(mutationID),
// TODO(knz): the dropped views are improperly qualified.
// See: https://github.com/cockroachdb/cockroach/issues/57735
TableName: tn.FQString(),
IndexName: string(idxName),
MutationID: uint32(mutationID),
CascadeDroppedViews: droppedViews,
})
}
12 changes: 8 additions & 4 deletions pkg/sql/drop_table.go
Original file line number Diff line number Diff line change
Expand Up @@ -143,9 +143,7 @@ func (n *dropTableNode) startExec(params runParams) error {
if err := params.p.logEvent(params.ctx,
droppedDesc.ID,
&eventpb.DropTable{
TableName: toDel.tn.FQString(),
// TODO(knz): the droppedViews are insufficiently qualified
// See: https://github.com/cockroachdb/cockroach/issues/57735
TableName: toDel.tn.FQString(),
CascadeDroppedViews: droppedViews,
}); err != nil {
return err
Expand Down Expand Up @@ -348,8 +346,14 @@ func (p *planner) dropTableImpl(
if err != nil {
return droppedViews, err
}

qualifiedView, err := p.getQualifiedTableName(ctx, viewDesc)
if err != nil {
return droppedViews, err
}

droppedViews = append(droppedViews, cascadedViews...)
droppedViews = append(droppedViews, viewDesc.Name)
droppedViews = append(droppedViews, qualifiedView.String())
}

err := p.removeTableComments(ctx, tableDesc)
Expand Down
8 changes: 7 additions & 1 deletion pkg/sql/drop_view.go
Original file line number Diff line number Diff line change
Expand Up @@ -223,12 +223,18 @@ func (p *planner) dropViewImpl(
if err != nil {
return cascadeDroppedViews, err
}

qualifiedView, err := p.getQualifiedTableName(ctx, dependentDesc)
if err != nil {
return cascadeDroppedViews, err
}

cascadedViews, err := p.dropViewImpl(ctx, dependentDesc, queueJob, "dropping dependent view", behavior)
if err != nil {
return cascadeDroppedViews, err
}
cascadeDroppedViews = append(cascadeDroppedViews, cascadedViews...)
cascadeDroppedViews = append(cascadeDroppedViews, dependentDesc.Name)
cascadeDroppedViews = append(cascadeDroppedViews, qualifiedView.String())
}
}

Expand Down
66 changes: 66 additions & 0 deletions pkg/sql/logictest/testdata/logic_test/event_log
Original file line number Diff line number Diff line change
Expand Up @@ -949,3 +949,69 @@ FROM system.eventlog
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 drop views.
subtest eventlog_dropped_views

statement ok
CREATE TABLE x (a INT PRIMARY KEY, b INT)

statement ok
CREATE VIEW y AS SELECT a FROM x

statement ok
CREATE VIEW z AS SELECT b FROM x

statement ok
DROP TABLE x CASCADE

query IT
SELECT "reportingID", info::JSONB - 'Timestamp' - 'DescriptorID'
FROM system.eventlog
WHERE "eventType" = 'drop_table'
ORDER BY "timestamp" DESC, info
LIMIT 1
----
1 {"CascadeDroppedViews": ["defaultdb.public.y", "defaultdb.public.z"], "EventType": "drop_table", "Statement": "DROP TABLE defaultdb.public.x CASCADE", "TableName": "defaultdb.public.x", "User": "root"}

statement ok
CREATE TABLE t (i INT PRIMARY KEY, INDEX (i))

statement ok
CREATE VIEW v AS (SELECT i FROM t@t_i_idx)

statement ok
CREATE VIEW w AS (SELECT I FROM t@t_i_idx)

statement ok
DROP INDEX t@t_i_idx CASCADE

query IT
SELECT "reportingID", info::JSONB - 'Timestamp' - 'DescriptorID'
FROM system.eventlog
WHERE "eventType" = 'drop_index'
ORDER BY "timestamp" DESC, info
LIMIT 1
----
1 {"EventType": "drop_index", "IndexName": "t_i_idx", "MutationID": 1, "Statement": "DROP INDEX defaultdb.public.t@t_i_idx CASCADE", "TableName": "defaultdb.public.t", "User": "root", "cascade_dropped_views": ["defaultdb.public.v", "defaultdb.public.w"]}

statement ok
CREATE TABLE x (a INT PRIMARY KEY, b INT)

statement ok
CREATE VIEW v AS SELECT b FROM x

statement ok
CREATE VIEW vv as SELECT b FROM x

statement ok
ALTER TABLE x DROP COLUMN b CASCADE

query IT
SELECT "reportingID", info::JSONB - 'Timestamp' - 'DescriptorID'
FROM system.eventlog
WHERE "eventType" = 'alter_table'
ORDER BY "timestamp" DESC, info
LIMIT 1
----
1 {"CascadeDroppedViews": ["defaultdb.public.v", "defaultdb.public.vv"], "EventType": "alter_table", "MutationID": 1, "Statement": "ALTER TABLE defaultdb.public.x DROP COLUMN b CASCADE", "TableName": "defaultdb.public.x", "User": "root"}
2 changes: 1 addition & 1 deletion pkg/sql/pgwire/testdata/pgtest/notice
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ Query {"String": "DROP INDEX t_x_idx"}
until crdb_only
CommandComplete
----
{"Severity":"NOTICE","Code":"00000","Message":"the data for dropped indexes is reclaimed asynchronously","Detail":"","Hint":"The reclamation delay can be customized in the zone configuration for the table.","Position":0,"InternalPosition":0,"InternalQuery":"","Where":"","SchemaName":"","TableName":"","ColumnName":"","DataTypeName":"","ConstraintName":"","File":"drop_index.go","Line":515,"Routine":"dropIndexByName","UnknownFields":null}
{"Severity":"NOTICE","Code":"00000","Message":"the data for dropped indexes is reclaimed asynchronously","Detail":"","Hint":"The reclamation delay can be customized in the zone configuration for the table.","Position":0,"InternalPosition":0,"InternalQuery":"","Where":"","SchemaName":"","TableName":"","ColumnName":"","DataTypeName":"","ConstraintName":"","File":"drop_index.go","Line":521,"Routine":"dropIndexByName","UnknownFields":null}
{"Type":"CommandComplete","CommandTag":"DROP INDEX"}

until noncrdb_only
Expand Down
10 changes: 8 additions & 2 deletions pkg/sql/resolver.go
Original file line number Diff line number Diff line change
Expand Up @@ -405,13 +405,19 @@ func (p *planner) getQualifiedTableName(
ctx context.Context, desc catalog.TableDescriptor,
) (*tree.TableName, error) {
dbDesc, err := p.Descriptors().GetImmutableDatabaseByID(ctx, p.txn, desc.GetParentID(),
tree.DatabaseLookupFlags{})
tree.DatabaseLookupFlags{
IncludeOffline: true,
IncludeDropped: true,
})
if err != nil {
return nil, err
}
schemaID := desc.GetParentSchemaID()
resolvedSchema, err := p.Descriptors().GetImmutableSchemaByID(ctx, p.txn, schemaID,
tree.SchemaLookupFlags{})
tree.SchemaLookupFlags{
IncludeOffline: true,
IncludeDropped: true,
})
if err != nil {
return nil, err
}
Expand Down

0 comments on commit 410188f

Please sign in to comment.