Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
131590: sql: ALTER COLUMN TYPE does not check partial index definitions r=Dedej-Bergin a=Dedej-Bergin

Previously we would allow ALTER COLUMN TYPE to succeed on a column even when we have a partial index on it.  This would cause issues down the line for example at insert and is inconsistent with postgres.  This fix now checks for partial index and errors out like postgres when your run the ALTER COLUMN TYPE command on a column with a partial index.

Fixes: cockroachdb#72456
Epic: CRDB-25314

Release note (bug fix): ALTER COLUMN TYPE now errors out when we have a partial index dependent on the column being altered.

Co-authored-by: Bergin Dedej <[email protected]>
  • Loading branch information
craig[bot] and Dedej-Bergin committed Oct 3, 2024
2 parents ac0b244 + 5047d8b commit 1922797
Show file tree
Hide file tree
Showing 4 changed files with 54 additions and 4 deletions.
8 changes: 7 additions & 1 deletion pkg/sql/alter_column_type.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,13 +62,19 @@ func AlterColumnType(
)
}
}
if err := schemaexpr.ValidateTTLExpressionDoesNotDependOnColumn(tableDesc, tableDesc.GetRowLevelTTL(), col, tn, op); err != nil {

if err := schemaexpr.ValidateTTLExpression(tableDesc, tableDesc.GetRowLevelTTL(), col, tn, op); err != nil {
return err
}

if err := schemaexpr.ValidateComputedColumnExpressionDoesNotDependOnColumn(tableDesc, col, objType, op); err != nil {
return err
}

if err := schemaexpr.ValidatePartialIndex(tableDesc, col, objType, op); err != nil {
return err
}

typ, err := tree.ResolveType(ctx, t.ToType, params.p.semaCtx.GetTypeResolver())
if err != nil {
return err
Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/alter_table.go
Original file line number Diff line number Diff line change
Expand Up @@ -1928,7 +1928,7 @@ func dropColumnImpl(
if err := schemaexpr.ValidateColumnHasNoDependents(tableDesc, colToDrop); err != nil {
return nil, err
}
if err := schemaexpr.ValidateTTLExpressionDoesNotDependOnColumn(tableDesc, rowLevelTTL, colToDrop, tn, "drop"); err != nil {
if err := schemaexpr.ValidateTTLExpression(tableDesc, rowLevelTTL, colToDrop, tn, "drop"); err != nil {
return nil, err
}

Expand Down
31 changes: 29 additions & 2 deletions pkg/sql/catalog/schemaexpr/expr.go
Original file line number Diff line number Diff line change
Expand Up @@ -463,9 +463,9 @@ func SanitizeVarFreeExpr(
return typedExpr, nil
}

// ValidateTTLExpressionDoesNotDependOnColumn verifies that the
// ValidateTTLExpression verifies that the
// ttl_expiration_expression, if any, does not reference the given column.
func ValidateTTLExpressionDoesNotDependOnColumn(
func ValidateTTLExpression(
tableDesc catalog.TableDescriptor,
rowLevelTTL *catpb.RowLevelTTL,
col catalog.Column,
Expand Down Expand Up @@ -505,6 +505,33 @@ func ValidateComputedColumnExpressionDoesNotDependOnColumn(
return nil
}

// ValidatePartialIndex verifies that we have no partial indexes
// that reference the column through the partial index's predicate.
func ValidatePartialIndex(
tableDesc catalog.TableDescriptor, dependentCol catalog.Column, objType, op string,
) error {
for _, idx := range tableDesc.AllIndexes() {
if idx.IsPartial() {
expr, err := parser.ParseExpr(idx.GetPredicate())
if err != nil {
return err
}

colIDs, err := ExtractColumnIDs(tableDesc, expr)
if err != nil {
return err
}

isReferencedByPredicate := colIDs.Contains(dependentCol.GetID())

if isReferencedByPredicate {
return sqlerrors.ColumnReferencedByPartialIndex(op, objType, string(dependentCol.ColName()), idx.GetName())
}
}
}
return nil
}

// ValidateTTLExpirationExpression verifies that the ttl_expiration_expression,
// if any, is valid according to the following rules:
// * type-checks as a TIMESTAMPTZ.
Expand Down
17 changes: 17 additions & 0 deletions pkg/sql/logictest/testdata/logic_test/alter_column_type
Original file line number Diff line number Diff line change
Expand Up @@ -1215,4 +1215,21 @@ t_int CREATE TABLE public.t_int (
statement ok
DROP TABLE t_int;

subtest partial_index_fails_with_error

statement ok
create table roach_village ( roach_id int primary key, name text, age smallint, legs smallint );

statement ok
create index idx on roach_village(age) where name = 'papa roach';

statement error pq: cannot alter type of column "name" because it is referenced by partial index "idx"
alter table roach_village alter column name set data type char(20);

statement ok
alter table roach_village alter column age set data type bigint;

statement ok
alter table roach_village alter column legs set data type bigint;

subtest end

0 comments on commit 1922797

Please sign in to comment.