sql/schemachanger: block dropping indexes that are the only source of…

… a column

Previously, because of a bug we could end up with secondary indexes
being the only thing that stored a column. This could lead to data
loss if that secondary index was dropped. To address this, this patch
will block dropping such indexes with a link to the technical advisory.

Informs: cockroachdb#106739

Release note (bug fix): Block dropping of indexes impacted by
technical advisory 99561 if data loss could occur.

pkg/sql/drop_index.go

@@ -138,6 +138,23 @@ func (n *dropIndexNode) startExec(params runParams) error {
 			return false
 		}
 
+		// Determine if any of the columns stored by this index are *not* kept
+		// in the primary index. Dropping these columns will lead to data loss.
+		if idx != nil {
+			primaryStoredColumns := tableDesc.GetPrimaryIndex().CollectPrimaryStoredColumnIDs()
+			secondaryStoredColumns := idx.CollectSecondaryStoredColumnIDs()
+			colsToRemove := catalog.TableColSet{}
+			for _, col := range tableDesc.AllColumns() {
+				if col.IsVirtual() {
+					colsToRemove.Add(col.GetID())
+				}
+			}
+			secondaryStoredColumns = secondaryStoredColumns.Difference(colsToRemove)
+			if missingColumns := secondaryStoredColumns.Difference(primaryStoredColumns); !missingColumns.Empty() {
+				return sqlerrors.NewSecondaryIndexDataLossError(idx.GetName())
+			}
+		}
+
 		// Drop expression index columns if they are not key columns in any
 		// other index. They cannot be referenced in constraints, computed
 		// columns, or other indexes, so they are safe to drop.

pkg/sql/logictest/testdata/logic_test/drop_index

@@ -366,3 +366,92 @@ subtest drop_primary_key
 
 statement error pgcode 0A000 cannot drop the primary index of a table using DROP INDEX
 CREATE TABLE drop_primary(); DROP INDEX drop_primary@drop_primary_pkey CASCADE;
+
+# We had a bug where after adding a column it could have wound up only in a
+# secondary index as storing if the index was created as a side effect of
+# ALTER PRIMARY KEY on 20.2 and 21.1. Simulate this corruption and ensure
+# if such an index exists to prevent data loss drops are blocked with a link
+# to the technical advisory.
+subtest drop_index_data_loss_block
+
+statement ok
+CREATE TABLE t_with_idx_data_loss (n INT8 PRIMARY KEY, j INT8);
+CREATE INDEX ON t_with_idx_data_loss(n) STORING (j);
+
+# Corrupt the primary index to stop storing j, so the only source is storing
+# is a secondary index.
+statement ok
+WITH
+	to_json
+		AS (
+			SELECT
+				id,
+				crdb_internal.pb_to_json(
+					'cockroach.sql.sqlbase.Descriptor',
+					descriptor,
+					false
+				)
+					AS d
+			FROM
+				system.descriptor
+			WHERE
+				id IN ('t_with_idx_data_loss'::regclass::oid::int,)
+		),
+	modified
+		AS (
+			SELECT
+				id,
+				json_set(
+					json_set(
+						json_remove_path(
+							json_remove_path(
+								d,
+								ARRAY[
+									'table',
+									'primaryIndex',
+									'storeColumnIds'
+								]
+							),
+							ARRAY[
+								'table',
+								'primaryIndex',
+								'storeColumnNames'
+							]
+						),
+						ARRAY['table', 'version'],
+						(
+							(d->'table'->>'version')::INT8
+							+ 1
+						)::STRING::JSONB
+					),
+					ARRAY['table', 'modificationTime'],
+					json_build_object(
+						'wallTime',
+						(
+							(
+								extract('epoch', now())
+								* 1000000
+							)::INT8
+							* 1000
+						)::STRING
+					)
+				)
+					AS d
+			FROM
+				to_json
+		)
+SELECT
+	crdb_internal.unsafe_upsert_descriptor(
+		id,
+		crdb_internal.json_to_pb(
+			'cockroach.sql.sqlbase.Descriptor',
+			d
+		),
+		false
+	)
+FROM
+	modified;
+
+# Drop INDEX will be blocked
+statement error pq: index t_with_idx_data_loss_n_idx cannot be safely dropped, since doing so will lose data in certain columns
+DROP INDEX t_with_idx_data_loss@t_with_idx_data_loss_n_idx;

pkg/sql/pgwire/testdata/pgtest/notice

@@ -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":529,"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":546,"Routine":"dropIndexByName","UnknownFields":null}
 {"Type":"CommandComplete","CommandTag":"DROP INDEX"}
 
 until noncrdb_only

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

@@ -20,6 +20,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
 	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scerrors"
 	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb"
+	"github.com/cockroachdb/cockroach/pkg/sql/sem/catid"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlerrors"
 	"github.com/cockroachdb/errors"
@@ -120,6 +121,43 @@ func dropAnIndex(
 	dropSecondaryIndex(b, index, dropBehavior, sie, toBeDroppedIndexElms)
 }
 
+// panicIfColumnsAreNotStoredElseWhere detects if dropping an index will lose a stored
+// column. Previously, we had an issue where new columns were accidentally
+// added into secondary indexes. This could lead to wrong results and data
+// loss, so for safety we are going to block dropping these secondary indexes.
+func panicIfColumnsAreNotStoredElseWhere(b BuildCtx, sie *scpb.SecondaryIndex) {
+	tblElts := b.QueryByID(sie.TableID)
+	secondaryIdxElts := tblElts.Filter(hasIndexIDAttrFilter(sie.IndexID))
+	_, _, indexName := scpb.FindIndexName(secondaryIdxElts)
+	_, _, pie := scpb.FindPrimaryIndex(b.QueryByID(sie.TableID))
+	primaryIdxElts := tblElts.Filter(hasIndexIDAttrFilter(pie.IndexID))
+
+	foundColumns := make(map[catid.ColumnID]struct{})
+	primaryIdxElts.ForEachElementStatus(func(current scpb.Status, target scpb.TargetStatus, e scpb.Element) {
+		idxColumn, ok := (e).(*scpb.IndexColumn)
+		if !ok {
+			return
+		}
+		foundColumns[idxColumn.ColumnID] = struct{}{}
+	})
+	secondaryIdxElts.ForEachElementStatus(func(current scpb.Status, target scpb.TargetStatus, e scpb.Element) {
+		idxColumn, ok := (e).(*scpb.IndexColumn)
+		if !ok {
+			return
+		}
+		columnElts := tblElts.Filter(hasColumnIDAttrFilter(idxColumn.ColumnID))
+		// Skip columns that are virtual.
+		_, _, columnType := scpb.FindColumnType(columnElts)
+		if columnType.IsVirtual {
+			return
+		}
+		// Check if the column is stored in primary index.
+		if _, found := foundColumns[idxColumn.ColumnID]; !found {
+			panic(sqlerrors.NewSecondaryIndexDataLossError(indexName.Name))
+		}
+	})
+}
+
 // dropSecondaryIndex is a helper to drop a secondary index which may be used
 // both in DROP INDEX and as a cascade from another operation.
 func dropSecondaryIndex(
@@ -129,6 +167,10 @@ func dropSecondaryIndex(
 	sie *scpb.SecondaryIndex,
 	toBeDroppedIndexElms ElementResultSet,
 ) {
+	// Sanity: We had a pretty severe bug with a data loss risk, prevent
+	// dropping of any indexes that are the sole source for given data.
+	panicIfColumnsAreNotStoredElseWhere(b, sie)
+
 	// Maybe drop dependent views.
 	// If CASCADE and there are "dependent" views (i.e. views that use this
 	// to-be-dropped index), then we will drop all dependent views and their

pkg/sql/sqlerrors/errors.go

@@ -373,3 +373,16 @@ func errHasCode(err error, code ...pgcode.Code) bool {
 	}
 	return false
 }
+
+// NewSecondaryIndexDataLossError creates an error if dropping a secondary
+// index could lead to losing a column.
+func NewSecondaryIndexDataLossError(indexName string) error {
+	return errors.WithIssueLink(errors.Newf("index %s cannot be safely dropped, "+
+		"since doing so will lose data in certain columns", indexName),
+		errors.IssueLink{
+			IssueURL: "https://www.cockroachlabs.com/docs/advisories/a99561",
+			Detail: "Dropping this secondary index may lead to data loss, please " +
+				"contact support.",
+		},
+	)
+}