sql: support ALTER INDEX … NOT VISIBLE

This commit adds the actual execution for ALTER INDEX … [VISIBLE | NOT VISIBLE].

Assists: cockroachdb#72576

See also: cockroachdb#84783

Release note (sql change): Altering an index to visible or not visible using
ALTER INDEX … VISIBLE | NOT VISIBLE is now supported.

docs/generated/eventlog.md

@@ -616,6 +616,31 @@ An event of type `alter_index` is recorded when an index is altered.
 | `MutationID` | The mutation ID for the asynchronous job that is processing the index update. | no |
 
 
+#### 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. The statement string contains a mix of sensitive and non-sensitive details (it is redactable). | partially |
+| `Tag` | The statement tag. This is separate from the statement string, since the statement string can contain sensitive information. The tag is guaranteed not to. | no |
+| `User` | The user account that triggered the event. The special usernames `root` and `node` are not considered sensitive. | depends |
+| `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. Application names starting with a dollar sign (`$`) are not considered sensitive. | no |
+| `PlaceholderValues` | The mapping of SQL placeholders to their values, for prepared statements. | yes |
+
+### `alter_index_visible`
+
+AlterIndex is recorded when an index visibility is altered.
+
+
+| Field | Description | Sensitive |
+|--|--|--|
+| `TableName` | The name of the table containing the affected index. | yes |
+| `IndexName` | The name of the affected index. | yes |
+| `NotVisible` | Set true if index is not visible. | no |
+
+
 #### Common fields
 
 | Field | Description | Sensitive |

pkg/sql/alter_index_visible.go

@@ -3,10 +3,15 @@ package sql
 import (
 	"context"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
-	"github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented"
-
+	"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/sem/tree"
+	"github.com/cockroachdb/cockroach/pkg/util/log/eventpb"
+	"github.com/cockroachdb/errors"
 )
 
 type alterIndexVisibleNode struct {
@@ -16,9 +21,45 @@ type alterIndexVisibleNode struct {
 }
 
 func (p *planner) AlterIndexVisible(ctx context.Context, stmt *tree.AlterIndexVisible) (planNode, error) {
-	return nil, unimplemented.Newf(
-		"Not Visible Index",
-		"altering an index to visible or not visible is not supported yet")
+	if err := checkSchemaChangeEnabled(
+		ctx,
+		p.ExecCfg(),
+		"ALTER INDEX VISIBILITY",
+	); err != nil {
+		return nil, err
+	}
+
+	// Check if the table actually exists. expandMutableIndexName returns the
+	// underlying table for the index. If no table is found and IfExists is false,
+	// error is returned. Otherwise, tableDesc returned is nil.
+	_, tableDesc, err := expandMutableIndexName(ctx, p, stmt.Index, !stmt.IfExists /* requireTable */)
+	if err != nil {
+		// Error if no table is found and IfExists is false.
+		return nil, err
+	}
+
+	if tableDesc == nil {
+		// Nothing needed if no table is found and IfExists is true.
+		return newZeroNode(nil /* columns */), nil
+	}
+
+	// Now we know this table exists, check if the index exists. FindIndexWithName
+	// returns error if none was found in the tableDesc.
+	idx, err := tableDesc.FindIndexWithName(string(stmt.Index.Index))
+	if err != nil {
+		if stmt.IfExists {
+			// Nothing needed if no index exists and IfExists is true.
+			return newZeroNode(nil /* columns */), nil
+		}
+		// Error if no index exists and IfExists is not specified.
+		return nil, pgerror.WithCandidateCode(err, pgcode.UndefinedObject)
+	}
+
+	if err := p.CheckPrivilege(ctx, tableDesc, privilege.CREATE); err != nil {
+		return nil, err
+	}
+
+	return &alterIndexVisibleNode{n: stmt, tableDesc: tableDesc, index: idx}, nil
 }
 
 // TODO (wenyihu6): Double check on whether we should have ReadingOwnWrites. I
@@ -28,9 +69,57 @@ func (p *planner) AlterIndexVisible(ctx context.Context, stmt *tree.AlterIndexVi
 // func (n *alterIndexVisibleNode) ReadingOwnWrites() {}
 
 func (n *alterIndexVisibleNode) startExec(params runParams) error {
-	return unimplemented.Newf(
-		"Not Visible Index",
-		"altering an index to visible or not visible is not supported yet")
+	if n.n.NotVisible {
+		if n.index.Primary() {
+			return errors.WithHint(
+				pgerror.Newf(pgcode.FeatureNotSupported, "primary index cannot be invisible"),
+				"instead, use ALTER TABLE ... ALTER PRIMARY KEY or ADD CONSTRAINT ... PRIMARY KEY "+
+					"and change the secondary index invisible",
+			)
+		}
+	}
+
+	if n.index.IsNotVisible() == n.n.NotVisible {
+		// Nothing needed if the index is already what they want.
+		str := ""
+		if n.n.NotVisible {
+			str = "not "
+		}
+		params.p.BufferClientNotice(
+			params.ctx,
+			pgnotice.Newf("index is already "+str+"visible"),
+		)
+		return nil
+	}
+
+	n.index.IndexDesc().NotVisible = n.n.NotVisible
+
+	if err := validateDescriptor(params.ctx, params.p, n.tableDesc); err != nil {
+		return err
+	}
+
+	if err := params.p.writeSchemaChange(
+		params.ctx, n.tableDesc, descpb.InvalidMutationID, tree.AsStringWithFQNames(n.n, params.Ann()),
+	); err != nil {
+		return err
+	}
+
+	// Warn against dropping an index if there exists a NotVisible index that may
+	// be used for constraint check behind the scene.
+	if notVisibleIndexNotice := tabledesc.ValidateNotVisibleIndexWithinTable(n.tableDesc); notVisibleIndexNotice != nil {
+		params.p.BufferClientNotice(
+			params.ctx,
+			notVisibleIndexNotice,
+		)
+	}
+
+	return params.p.logEvent(params.ctx,
+		n.tableDesc.ID,
+		&eventpb.AlterIndexVisible{
+			TableName:  n.n.Index.Table.FQString(),
+			IndexName:  n.index.GetName(),
+			NotVisible: n.n.NotVisible,
+		})
 }
 func (n *alterIndexVisibleNode) Next(runParams) (bool, error) { return false, nil }
 func (n *alterIndexVisibleNode) Values() tree.Datums          { return tree.Datums{} }

pkg/sql/opt/exec/execbuilder/testdata/not_visible_index

@@ -894,21 +894,116 @@ DROP TABLE parent
 
 ############################################################################
 # The following tests check for ALTER INDEX ... VISIBLE | NOT VISIBLE. 
+# - if an index is already what they want, logs a notice
+# - altering a primary index to not visible throws an error.
+# - altering a unique index to not visible logs a notice.
+# - altering a unique index to not visible logs a notice.
 ############################################################################
 statement ok
-CREATE TABLE t (p INT PRIMARY KEY, INDEX idx (p) VISIBLE)
+CREATE TABLE t (p INT PRIMARY KEY, other INT, UNIQUE (p), UNIQUE INDEX u_idx (p), INDEX idx (other))
+
+# Primary index cannot be invisible.
+statement error pq: primary index cannot be invisible
+ALTER INDEX t_pkey NOT VISIBLE
+
+# Error if index does not exist and no IF EXISTS.
+statement error pq: index "nonexistent_idx" does not exist
+ALTER INDEX nonexistent_idx NOT VISIBLE
+
+# No error if index does not exist and IF EXISTS is specified.
+statement ok
+ALTER INDEX IF EXISTS nonexistent_idx NOT VISIBLE
+
+# If an index is already what they want, logs a notice. 
+query T noticetrace
+ALTER INDEX t_p_key VISIBLE
+----
+NOTICE: index is already visible
+
+# Altering a unique constraint index to not visible logs a notice. 
+query T noticetrace
+ALTER INDEX t_p_key NOT VISIBLE
+----
+NOTICE: invisible indexes may still be used for unique or foreign key constraint check, so the query plan may be different from dropping the index completely.
+
+# Altering a unique index to not visible logs a notice. 
+query T noticetrace
+ALTER INDEX u_idx NOT VISIBLE
+----
+NOTICE: invisible indexes may still be used for unique or foreign key constraint check, so the query plan may be different from dropping the index completely.
+
+# Check if the index is indeed invisible. 
+query T
+EXPLAIN SELECT * FROM t WHERE other > 2
+----
+distribution: local
+vectorized: true
+·
+• scan
+  missing stats
+  table: t@idx
+  spans: [/3 - ]
+
+query T noticetrace
+ALTER INDEX idx NOT VISIBLE
+----
+
+# SELECT ignores idx.
+query T
+EXPLAIN SELECT * FROM t WHERE other > 2
+----
+distribution: local
+vectorized: true
+·
+• filter
+│ filter: other > 2
+│
+└── • scan
+      missing stats
+      table: t@t_pkey
+      spans: FULL SCAN
+
+statement ok
+CREATE TABLE child1 (c INT PRIMARY KEY, p INT NOT NULL REFERENCES t(p), INDEX idx_visible (p))
+
+# Alter an index in a child table to not visible logs a warning.
+query T noticetrace
+ALTER INDEX idx_visible NOT VISIBLE
+----
+NOTICE: invisible indexes may still be used for unique or foreign key constraint check, so the query plan may be different from dropping the index completely.
+
+statement ok
+CREATE TABLE child2 (c INT PRIMARY KEY, p INT, INDEX idx_visible (p))
+
+query T noticetrace
+ALTER TABLE child2 ADD CONSTRAINT fk FOREIGN KEY (p) REFERENCES t(p);
+----
+
+# Check ambiguous index error. 
+statement error pq: index name "idx_visible" is ambiguous
+ALTER INDEX idx_visible NOT VISIBLE
+
+# Alter an index in a child table to not visible logs a warning.
+query T noticetrace
+ALTER INDEX child2@idx_visible NOT VISIBLE
+----
+NOTICE: invisible indexes may still be used for unique or foreign key constraint check, so the query plan may be different from dropping the index completely.
 
 query TTB
 SELECT index_name, column_name, visible FROM [SHOW INDEX FROM t] ORDER BY index_name, seq_in_index
 ----
-idx     p  true
-t_pkey  p  true
+idx      other  false
+idx      p      false
+t_p_key  p      false
+t_pkey   p      true
+t_pkey   other  true
+u_idx    p      false
 
-statement error pq: unimplemented: altering an index to visible or not visible is not supported yet
-ALTER INDEX idx NOT VISIBLE
+statement ok
+DROP TABLE child2
 
-statement error pq: unimplemented: altering an index to visible or not visible is not supported yet
-ALTER INDEX idx VISIBLE
+statement ok
+DROP TABLE child1
 
 statement ok
 DROP TABLE t

pkg/util/log/eventpb/ddl_events.proto

@@ -325,6 +325,18 @@ message AlterIndex {
   uint32 mutation_id = 5 [(gogoproto.customname) = "MutationID", (gogoproto.jsontag) = ",omitempty"];
 }
 
+// AlterIndex is recorded when an index visibility is altered.
+message AlterIndexVisible {
+  CommonEventDetails common = 1 [(gogoproto.nullable) = false, (gogoproto.jsontag) = "", (gogoproto.embed) = true];
+  CommonSQLEventDetails sql = 2 [(gogoproto.nullable) = false, (gogoproto.jsontag) = "", (gogoproto.embed) = true];
+  // The name of the table containing the affected index.
+  string table_name = 3 [(gogoproto.jsontag) = ",omitempty"];
+  // The name of the affected index.
+  string index_name = 4 [(gogoproto.jsontag) = ",omitempty"];
+  // Set true if index is not visible.
+  bool not_visible = 5 [(gogoproto.jsontag) = ",omitempty"];
+}
+
 
 // CreateView is recorded when a view is created.
 message CreateView {