Skip to content

Commit

Permalink
sql/schemachanger: drop index notices shouldn't always be generated
Browse files Browse the repository at this point in the history
Previously, the declarative schema changer notices
were always generated even if no indexes were dropped.
This was inadequate because some of these should have
been reported if any work was done. To address this, this
patch will detect if any indexes were actually dropped.

Release note: None
  • Loading branch information
fqazi committed Dec 15, 2022
1 parent b460689 commit c276451
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 12 deletions.
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","SeverityUnlocalized":"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":522,"Routine":"dropIndexByName","UnknownFields":null}
{"Severity":"NOTICE","SeverityUnlocalized":"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":63,"Routine":"DropIndex","UnknownFields":null}
{"Type":"CommandComplete","CommandTag":"DROP INDEX"}

until noncrdb_only
Expand Down
25 changes: 14 additions & 11 deletions pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_index.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,38 +59,40 @@ func DropIndex(b BuildCtx, n *tree.DropIndex) {
pgnotice.Newf("CONCURRENTLY is not required as all indexes are dropped concurrently"))
}

var anyIndexesDropped bool
for _, index := range n.IndexList {
dropAnIndex(b, index, n.IfExists, n.DropBehavior)

anyIndexesDropped = dropAnIndex(b, index, n.IfExists, n.DropBehavior) || anyIndexesDropped
// Increment subwork ID so we know exactly which portion in
// a `DROP INDEX index1, index2, ...` statement is responsible
// for the creation of the targets.
b.IncrementSubWorkID()
b.IncrementSchemaChangeDropCounter("index")

}
b.EvalCtx().ClientNoticeSender.BufferClientNotice(
b,
errors.WithHint(
pgnotice.Newf("the data for dropped indexes is reclaimed asynchronously"),
"The reclamation delay can be customized in the zone configuration for the table.",
),
)
if anyIndexesDropped {
b.EvalCtx().ClientNoticeSender.BufferClientNotice(
b,
errors.WithHint(
pgnotice.Newf("the data for dropped indexes is reclaimed asynchronously"),
"The reclamation delay can be customized in the zone configuration for the table.",
),
)
}
}

// dropAnIndex resolves `index` and mark its constituent elements as ToAbsent
// in the builder state enclosed by `b`.
func dropAnIndex(
b BuildCtx, indexName *tree.TableIndexName, ifExists bool, dropBehavior tree.DropBehavior,
) {
) (indexDropped bool) {
toBeDroppedIndexElms := b.ResolveIndexByName(indexName, ResolveParams{
IsExistenceOptional: ifExists,
RequiredPrivilege: privilege.CREATE,
})
if toBeDroppedIndexElms == nil {
// Attempt to resolve this index failed but `IF EXISTS` is set.
b.MarkNameAsNonExistent(&indexName.Table)
return
return false
}
// Panic if dropping primary index.
_, _, pie := scpb.FindPrimaryIndex(toBeDroppedIndexElms)
Expand Down Expand Up @@ -118,6 +120,7 @@ func dropAnIndex(
))
}
dropSecondaryIndex(b, indexName, dropBehavior, sie)
return true
}

// dropSecondaryIndex is a helper to drop a secondary index which may be used
Expand Down

0 comments on commit c276451

Please sign in to comment.