Merge #108047

108047: sql: emit NOTICEs when implicitly dropping indexes r=chrisseto a=chrisseto

**sql: emit NOTICEs when implicitly dropping indexes**
Previously, users would have to either know ahead of time or later
discover if an index would be implicitly dropped by dropping a column
that the index referenced.

At best, this was mildly surprising. At worst, workloads could be
dramatically impacted without much indication as to why.

To ensure implicit drops do not go unnoticed, this commit updates `ALTER
TABLE ... DROP COLUMN ...` to emit NOTICEs for each implicitly dropped
index.

Fixes: #106907
Release note (sql change): NOTICEs are now emitted for each index
dropped by an `ALTER TABLE ... DROP COLUMN ...` statement.

**sql: warn users that DROP COLUMN may drop indexes**
Previously, `ALTER TABLE ... DROP COLUMN ...` statements would warn
users that all data held by that column would be destroyed when
`sql_safe_updates = on`. Unlike the [documentation for `DROP
COLUMN`](https://www.cockroachlabs.com/docs/stable/alter-table#drop-column),
this message does not indicate that indexes may be dropped.

This commit extends the warning to also inform users that any indexes
referencing that column will also be dropped to match the warnings in
our documentation.

Epic: none
Informs: #106907

Release note: (sql change): Attempting to drop a column when
`sql_safe_updates = on` now additionally warns users that indexes
referencing that column will be automatically dropped.


Co-authored-by: Chris Seto <[email protected]>

pkg/sql/alter_table.go

@@ -488,7 +488,7 @@ func (n *alterTableNode) startExec(params runParams) error {
 		case *tree.AlterTableDropColumn:
 			if params.SessionData().SafeUpdates {
 				err := pgerror.DangerousStatementf("ALTER TABLE DROP COLUMN will " +
-					"remove all data in that column")
+					"remove all data in that column and drop any indexes that reference that column")
 				if !params.extendedEvalCtx.TxnIsSingleStmt {
 					err = errors.WithIssueLink(err, errors.IssueLink{
 						IssueURL: "https://github.com/cockroachdb/cockroach/issues/46541",
@@ -1718,6 +1718,11 @@ func dropColumnImpl(
 	}
 
 	for _, idxName := range idxNamesToDelete {
+		params.EvalContext().ClientNoticeSender.BufferClientNotice(params.ctx, pgnotice.Newf(
+			"dropping index %q which depends on column %q",
+			idxName,
+			colToDrop.ColName(),
+		))
 		jobDesc := fmt.Sprintf(
 			"removing index %q dependent on column %q which is being dropped; full details: %s",
 			idxName,

pkg/sql/logictest/testdata/logic_test/alter_table

@@ -391,7 +391,9 @@ ALTER TABLE t DROP COLUMN IF EXISTS q
 statement error cannot drop column "e" because view "v" depends on it
 ALTER TABLE t DROP COLUMN IF EXISTS e
 
-statement ok
+# Negative test to assert that NOTICEs are NOT emitted if there are no indexes
+# being dropped.
+statement notice ^$
 ALTER TABLE t DROP COLUMN IF EXISTS e CASCADE
 
 statement ok
@@ -403,12 +405,16 @@ CREATE TABLE o (gf INT REFERENCES t (g), h INT, i INT, INDEX oi (i), INDEX oh (h
 statement error "t_g_key" is referenced by foreign key from table "o"
 ALTER TABLE t DROP COLUMN g
 
-statement ok
+statement notice NOTICE: dropping index "t_g_key" which depends on column "g"
 ALTER TABLE t DROP COLUMN g CASCADE
 
 # Dropping columns that are indexed or stored in indexes drops those indexes
-# too.
-statement ok
+# too. Excuse the nasty regex, the legacy schema changer emits additional
+# NOTICEs about GCing. We're not testing for that, so we just "eat" them.
+# Example: "NOTICE: the data for dropped indexes is reclaimed
+# asynchronously\nHINT: The reclamation delay can be customized in the zone
+# configuration for the table."
+statement notice (NOTICE: dropping index "(oh|oih)" which depends on column "h"(\nNOTICE: .+\nHINT: .+\n)?\n?){2}
 ALTER TABLE o DROP COLUMN h
 
 query TTBITTTBBBF colnames,rowsort

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_drop_column.go

@@ -18,6 +18,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/parser"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
+	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgnotice"
 	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
 	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scerrors"
 	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb"
@@ -53,7 +54,7 @@ func checkSafeUpdatesForDropColumn(b BuildCtx) {
 		return
 	}
 	err := pgerror.DangerousStatementf("ALTER TABLE DROP COLUMN will " +
-		"remove all data in that column")
+		"remove all data in that column and drop any indexes that reference that column")
 	if !b.EvalCtx().TxnIsSingleStmt {
 		err = errors.WithIssueLink(err, errors.IssueLink{
 			IssueURL: "https://github.com/cockroachdb/cockroach/issues/46541",
@@ -227,6 +228,11 @@ func dropColumn(
 				Table: *tn,
 				Index: tree.UnrestrictedName(indexName.Name),
 			}
+			b.EvalCtx().ClientNoticeSender.BufferClientNotice(b, pgnotice.Newf(
+				"dropping index %q which depends on column %q",
+				indexName.Name,
+				cn.Name,
+			))
 			dropSecondaryIndex(b, &name, behavior, e)
 		case *scpb.View:
 			if behavior != tree.DropCascade {