sql: notices for NotVisible Indexes

Optimizer now supports creating invisible indexes after this
[PR](cockroachdb#85794). An important use case
for not visible indexes is to test the behaviour of dropping an index by marking
the index invisible. However, there are certain cases where users cannot expect
dropping an index to behave exactly the same as marking an index invisible. More
specifically, NotVisible indexes may still be used to police unique or foreign
key constraint check behind the scene. In those cases, dropping the index might
behave different from marking the index invisible. Prior to this commit, users
do not know about this without reading the documentation. This commit adds some
user-friendly notices when users are dropping or changing a not visible index
that might be helpful for constraint check.

There are two cases where we are giving this notice: 1. if this index is unique.
2. if this index is on child table and may help with FK check.

More details on how this decision was made in
docs/RFCS/20220628_invisible_index.md.

Assists: cockroachdb#72576

See also: cockroachdb#85794

Release justification: low risk to the existing functionality; this commit just
adds notices.

Release note: none

pkg/sql/alter_index_visible.go

@@ -79,6 +79,17 @@ func (n *alterIndexVisibleNode) startExec(params runParams) error {
 		return pgerror.Newf(pgcode.FeatureNotSupported, "primary index cannot be invisible")
 	}
 
+	// Warn if this invisible index may still be used to enforce constraint check
+	// behind the scene.
+	if n.n.NotVisible {
+		if notVisibleIndexNotice := tabledesc.ValidateNotVisibleIndex(n.index, n.tableDesc); notVisibleIndexNotice != nil {
+			params.p.BufferClientNotice(
+				params.ctx,
+				notVisibleIndexNotice,
+			)
+		}
+	}
+
 	if n.index.IsNotVisible() == n.n.NotVisible {
 		// Nothing needed if the index is already what they want.
 		return nil

pkg/sql/catalog/descpb/index.go

@@ -64,6 +64,16 @@ func (desc *IndexDescriptor) FillColumns(elems tree.IndexElemList) error {
 	return nil
 }
 
+// IsHelpfulOriginIndex returns whether the index may be a helpful index for
+// performing foreign key checks and cascades for a foreign key with the given
+// origin columns. Given the originColIDs for foreign key constraint, the index
+// could be useful for FK check if the first column covered by the index is
+// present in FK constraint columns.
+func (desc *IndexDescriptor) IsHelpfulOriginIndex(originColIDs ColumnIDs) bool {
+	isHelpfulOriginIndex := len(desc.KeyColumnIDs) > 0 && originColIDs.Contains(desc.KeyColumnIDs[0])
+	return !desc.IsPartial() && isHelpfulOriginIndex
+}
+
 // IsValidOriginIndex returns whether the index can serve as an origin index for a foreign
 // key constraint with the provided set of originColIDs.
 func (desc *IndexDescriptor) IsValidOriginIndex(originColIDs ColumnIDs) bool {

pkg/sql/catalog/table_elements.go

@@ -151,6 +151,7 @@ type Index interface {
 	GetShardColumnName() string
 
 	IsValidOriginIndex(originColIDs descpb.ColumnIDs) bool
+	IsHelpfulOriginIndex(originColIDs descpb.ColumnIDs) bool
 	IsValidReferencedUniqueConstraint(referencedColIDs descpb.ColumnIDs) bool
 
 	GetPartitioning() Partitioning

pkg/sql/catalog/tabledesc/BUILD.bazel

@@ -41,6 +41,7 @@ go_library(
         "//pkg/sql/parser",
         "//pkg/sql/pgwire/pgcode",
         "//pkg/sql/pgwire/pgerror",
+        "//pkg/sql/pgwire/pgnotice",
         "//pkg/sql/privilege",
         "//pkg/sql/rowenc",
         "//pkg/sql/schemachanger/scpb",

pkg/sql/catalog/tabledesc/index.go

@@ -142,6 +142,13 @@ func (w index) IsValidOriginIndex(originColIDs descpb.ColumnIDs) bool {
 	return w.desc.IsValidOriginIndex(originColIDs)
 }
 
+// IsHelpfulOriginIndex returns whether the index may be a helpful index for
+// performing foreign key checks and cascades for a foreign key with the given
+// origin columns.
+func (w index) IsHelpfulOriginIndex(originColIDs descpb.ColumnIDs) bool {
+	return w.desc.IsHelpfulOriginIndex(originColIDs)
+}
+
 // IsValidReferencedUniqueConstraint returns whether the index can serve as a
 // referenced index for a foreign  key constraint with the provided set of
 // referencedColumnIDs.

pkg/sql/catalog/tabledesc/validate.go

@@ -27,6 +27,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/rowenc"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/types"
@@ -760,6 +761,42 @@ func (desc *wrapper) ValidateSelf(vea catalog.ValidationErrorAccumulator) {
 	ValidateOnUpdate(desc, vea.Report)
 }
 
+// ValidateNotVisibleIndex returns a notice when dropping the given index may
+// behave differently than marking the index invisible. NotVisible indexes may
+// still be used to police unique or foreign key constraint check behind the
+// scene. Hence, dropping the index might behave different from marking the
+// index invisible. There are three cases where this might happen:
+// Case 1: If the index is unique,
+// - Sub case 1: if the given tableDes is a parent table and this index could be
+// useful for FK check on the parent table.
+// - Sub case 2: otherwise, a unique index may only be useful for unique
+// constraint check. These first two cases can be covered by just checking
+// whether the index is unique.
+// Case 2: if the given tableDesc is a child table and this index could be
+// helpful for FK check in the child table. Note that we can only decide if an
+// index is currently useful for FK check on a child table. It is possible that
+// the user adds FK constraint later and this invisible index becomes useful. No
+// notices would be given at that point.
+func ValidateNotVisibleIndex(
+	index catalog.Index, tableDesc catalog.TableDescriptor,
+) pgnotice.Notice {
+	notices := pgnotice.Newf("queries may still use not visible indexes to enforce unique and foreign key constraints")
+	if index.IsUnique() {
+		// Case 1: The given index is a unique index.
+		return notices
+	}
+
+	notice := tableDesc.ForeachOutboundFK(func(fk *descpb.ForeignKeyConstraint) error {
+		// Case 2: The given index is an index on a child table that may be useful
+		// for FK check.
+		if index.IsHelpfulOriginIndex(fk.OriginColumnIDs) {
+			return notices
+		}
+		return nil
+	})
+	return notice
+}
+
 // ValidateOnUpdate returns an error if there is a column with both a foreign
 // key constraint and an ON UPDATE expression, nil otherwise.
 func ValidateOnUpdate(desc catalog.TableDescriptor, errReportFn func(err error)) {

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

@@ -7,7 +7,14 @@
 # - Check invisible index feature using EXPLAIN
 
 statement ok
-CREATE TABLE t1 (k INT PRIMARY KEY, v INT, i INT, INDEX idx_v_visible(v) VISIBLE, INDEX idx_i_invisible(i) NOT VISIBLE, FAMILY (k, v, i))
+CREATE TABLE t1 (
+  k INT PRIMARY KEY,
+  v INT,
+  i INT,
+  INDEX idx_v_visible(v) VISIBLE,
+  INDEX idx_i_invisible(i) NOT VISIBLE,
+  FAMILY (k, v, i)
+)
 
 statement ok
 CREATE UNIQUE INDEX idx_v_invisible ON t1(v) NOT VISIBLE
@@ -549,7 +556,12 @@ DROP TABLE t2
 # Check Invisible Inverted Index and Partial Inverted Index.
 ##################################################################################
 statement ok
-CREATE TABLE t1 (id INT, data JSONB, geom GEOMETRY, INVERTED INDEX idx_geom_visible(geom) VISIBLE);
+CREATE TABLE t1 (
+  id INT,
+  data JSONB,
+  geom GEOMETRY,
+  INVERTED INDEX idx_geom_visible(geom) VISIBLE
+);
 
 # idx_geom_visible is chosen because it is visible.
 query T
@@ -614,7 +626,11 @@ vectorized: true
       spans: FULL SCAN
 
 statement ok
-CREATE TABLE t2 (id INT, geom2 GEOMETRY, INVERTED INDEX idx_geom2_invisible(geom2) NOT VISIBLE)
+CREATE TABLE t2 (
+  id INT,
+  geom2 GEOMETRY,
+  INVERTED INDEX idx_geom2_invisible(geom2) NOT VISIBLE
+)
 
 # Force INVERTED JOIN will result in error because idx_geom2_invisible is invisible.
 statement error could not produce a query plan conforming to the INVERTED JOIN hint
@@ -798,7 +814,11 @@ statement ok
 CREATE TABLE parent (p INT PRIMARY KEY)
 
 statement ok
-CREATE TABLE child (c INT PRIMARY KEY, p INT NOT NULL REFERENCES parent(p), INDEX c_idx_invisible (p) NOT VISIBLE)
+CREATE TABLE child (
+  c INT PRIMARY KEY,
+  p INT NOT NULL REFERENCES parent(p),
+  INDEX c_idx_invisible (p) NOT VISIBLE
+)
 
 # Part 1: When parent deletes or update, invisible indexes on the child table will be used.
 # c_idx_invisible is invisible when no FK is involved (delete on a child table
@@ -997,10 +1017,18 @@ statement ok
 CREATE TABLE parent (p INT PRIMARY KEY, other INT)
 
 statement ok
-CREATE TABLE child_delete (c INT PRIMARY KEY, p INT NOT NULL REFERENCES parent(p) ON DELETE CASCADE, INDEX c_delete_idx_invisible (p) NOT VISIBLE)
+CREATE TABLE child_delete (
+  c INT PRIMARY KEY,
+  p INT NOT NULL REFERENCES parent(p) ON DELETE CASCADE,
+  INDEX c_delete_idx_invisible (p) NOT VISIBLE
+)
 
 statement ok
-CREATE TABLE child_update (c INT PRIMARY KEY, p INT NOT NULL REFERENCES parent(p) ON UPDATE CASCADE, INDEX c_update_idx_invisible (p) NOT VISIBLE)
+CREATE TABLE child_update (
+  c INT PRIMARY KEY,
+  p INT NOT NULL REFERENCES parent(p) ON UPDATE CASCADE,
+  INDEX c_update_idx_invisible (p) NOT VISIBLE
+)
 
 # c_update_idx_invisible on child table is used for constraint check (delete on
 # a parent table requires FK check). This triggers delete cascade fast path since
@@ -1163,7 +1191,12 @@ subtest alter_index_visibility
 
 # The following tests check error and notices.
 statement ok
-CREATE TABLE t1 (c INT PRIMARY KEY, other INT NOT NULL, INDEX idx_visible (other) VISIBLE, INDEX idx_invisible (other) NOT VISIBLE)
+CREATE TABLE t1 (
+  c INT PRIMARY KEY,
+  other INT NOT NULL,
+  INDEX idx_visible (other) VISIBLE,
+  INDEX idx_invisible (other) NOT VISIBLE
+)
 
 query TTBITTBBB colnames
 SELECT * FROM [SHOW INDEX FROM t1]
@@ -1374,3 +1407,115 @@ vectorized: true
 
 statement ok
 DROP TABLE t
+
+############################################################################
+# We should log notices when dropping an invisible index might be different from marking an index invisible.
+############################################################################
+subtest not_visible_notices
+
+statement ok
+CREATE TABLE t (p INT PRIMARY KEY, INDEX idx_invisible (p))
+
+statement ok
+CREATE UNIQUE INDEX idx_invisible_unique ON t(p) VISIBLE
+
+# Notices if changing a unique index to NotVisible.
+query T noticetrace
+ALTER INDEX idx_invisible_unique NOT VISIBLE
+----
+NOTICE: queries may still use not visible indexes to enforce unique and foreign key constraints
+
+# No notices if changing a normal index to NotVisible.
+query T noticetrace
+ALTER INDEX idx_invisible NOT VISIBLE
+----
+
+statement ok
+DROP TABLE t
+
+statement ok
+CREATE TABLE parent (p INT PRIMARY KEY)
+
+statement ok
+CREATE TABLE child (
+  c INT PRIMARY KEY,
+  col1 INT,
+  p INT,
+  FOREIGN KEY (p, col1) REFERENCES parent(p, p),
+  INDEX c_idx_invisible_helpful1 (p, c),
+  INDEX c_idx_invisible_helpful2 (p, c, col1),
+  INDEX c_idx_invisible_helpful3 (col1, c, p),
+  INDEX c_idx_invisible_not_helpful (c, p)
+)
+
+# Indexes on child table is helpful as long as index's first column is present
+# in the FK constraint columns.
+# Notices if the index on the child table is helpful for FK check.
+query T noticetrace
+ALTER INDEX c_idx_invisible_helpful1 NOT VISIBLE
+----
+NOTICE: queries may still use not visible indexes to enforce unique and foreign key constraints
+
+# Notices if the index on the child table is helpful for FK check.
+query T noticetrace
+ALTER INDEX c_idx_invisible_helpful2 NOT VISIBLE
+----
+NOTICE: queries may still use not visible indexes to enforce unique and foreign key constraints
+
+# Notices if the index on the child table is helpful for FK check.
+query T noticetrace
+ALTER INDEX c_idx_invisible_helpful3 NOT VISIBLE
+----
+NOTICE: queries may still use not visible indexes to enforce unique and foreign key constraints
+
+# Index(c, p) is not helpful because c is not present in the FK constraint
+# origin columns. No notices if the index on the child table is not helpful for
+# FK check.
+query T noticetrace
+ALTER INDEX c_idx_invisible_not_helpful NOT VISIBLE
+----
+
+statement ok
+DROP TABLE child
+
+statement ok
+DROP TABLE parent
+
+statement ok
+CREATE TABLE parent (p INT PRIMARY KEY, c INT, d INT)
+
+statement ok
+CREATE UNIQUE INDEX p_unique_fk ON parent(c)
+
+statement ok
+CREATE UNIQUE INDEX p_unique_fk_duplicate ON parent(c)
+
+statement ok
+CREATE UNIQUE INDEX p_unique_only ON parent(d)
+
+statement ok
+CREATE TABLE child (c INT PRIMARY KEY, p INT NOT NULL REFERENCES parent(c))
+
+# Notices if the index on the parent table is helpful for both unique and FK check.
+query T noticetrace
+ALTER INDEX p_unique_fk NOT VISIBLE
+----
+NOTICE: queries may still use not visible indexes to enforce unique and foreign key constraints
+
+# Still gives notices if there exists two indexes that can be helpful for FK check.
+query T noticetrace
+ALTER INDEX p_unique_fk_duplicate NOT VISIBLE
+----
+NOTICE: queries may still use not visible indexes to enforce unique and foreign key constraints
+
+# Notices if the index on the parent table is helpful just for unique check.
+query T noticetrace
+ALTER INDEX p_unique_only NOT VISIBLE
+----
+NOTICE: queries may still use not visible indexes to enforce unique and foreign key constraints
+
+statement ok
+DROP TABLE child
+
+statement ok
+DROP TABLE parent