diff --git a/pkg/sql/drop_index.go b/pkg/sql/drop_index.go index 55418e720ced..9f14cc052b51 100644 --- a/pkg/sql/drop_index.go +++ b/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. diff --git a/pkg/sql/logictest/testdata/logic_test/drop_index b/pkg/sql/logictest/testdata/logic_test/drop_index index f50ffc8e191c..66e7446b72d1 100644 --- a/pkg/sql/logictest/testdata/logic_test/drop_index +++ b/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; diff --git a/pkg/sql/pgwire/testdata/pgtest/notice b/pkg/sql/pgwire/testdata/pgtest/notice index f1906b9de6f6..8a78ae6b0a25 100644 --- a/pkg/sql/pgwire/testdata/pgtest/notice +++ b/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 diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_index.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_index.go index f381ea05e95b..7e437a81dd5e 100644 --- a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_index.go +++ b/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 diff --git a/pkg/sql/sqlerrors/errors.go b/pkg/sql/sqlerrors/errors.go index dfe0e35ac305..6ba618814a11 100644 --- a/pkg/sql/sqlerrors/errors.go +++ b/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.", + }, + ) +}