From 78099d549558cf70363d7dbc217fedc5f6530254 Mon Sep 17 00:00:00 2001 From: arulajmani Date: Mon, 1 Mar 2021 13:56:23 -0500 Subject: [PATCH] sql: validate against array type usages when dropping enum values Previously, when dropping an enum value, we weren't validating if the enum value was used in a column of array type. This patch fixes the bug. Fixes #60004 Release justification: bug fix to new functionality Release note: None --- .../logictest/testdata/logic_test/alter_type | 78 ++++++++++++ pkg/sql/type_change.go | 119 +++++++++++++++--- 2 files changed, 182 insertions(+), 15 deletions(-) diff --git a/pkg/sql/logictest/testdata/logic_test/alter_type b/pkg/sql/logictest/testdata/logic_test/alter_type index 84d2b851220b..baa424b68cf6 100644 --- a/pkg/sql/logictest/testdata/logic_test/alter_type +++ b/pkg/sql/logictest/testdata/logic_test/alter_type @@ -496,3 +496,81 @@ SELECT ARRAY['c']::_ab_58710; statement error invalid input value for enum ab_58710: "a" SELECT ARRAY['a']::_ab_58710 + +subtest regression_60004_basic + +statement ok +CREATE TYPE enum_60004 AS ENUM ('a', 'b', 'c') + +statement ok +CREATE TABLE t_60004 (k INT PRIMARY KEY, v enum_60004[]) + +statement ok +INSERT INTO t_60004 VALUES (1, ARRAY['a']) + +statement error could not remove enum value "a" as it is being used by table "t_60004" +ALTER TYPE enum_60004 DROP VALUE 'a' + +# Not used in a row, so this DROP should be fine. +statement ok +ALTER TYPE enum_60004 DROP VALUE 'b' + +statement ok +CREATE VIEW v_60004 AS SELECT ARRAY['c']:::_enum_60004 AS v; + +statement error count-array-type-value-usage: enum value "c" is not yet public +ALTER TYPE enum_60004 DROP VALUE 'c' + +subtest regression_60004_complex +# Setup +statement ok +CREATE TYPE alphabets_60004 AS ENUM ('a', 'b', 'c', 'd') + +statement ok +CREATE TABLE using_alphabets_60004(k INT PRIMARY KEY, v1 alphabets_60004[], v2 alphabets_60004[]) + +statement ok +INSERT INTO using_alphabets_60004 VALUES (1, ARRAY['a', 'b', 'c'], ARRAY['a','b']), (2, ARRAY['a', 'b', 'c'], ARRAY['a','d']) + +statement ok +CREATE TABLE using_alphabets2_60004(k INT PRIMARY KEY, v1 alphabets_60004, v2 alphabets_60004[]) + +statement ok +INSERT INTO using_alphabets2_60004 VALUES (1, 'a', ARRAY['b', 'a']) + +statement error could not remove enum value "d" as it is being used by table "using_alphabets_60004" +ALTER TYPE alphabets_60004 DROP VALUE 'd' + +# Remove the row that uses 'd' in it and then try dropping. +statement ok +DELETE FROM using_alphabets_60004 WHERE k = 2 + +statement ok +ALTER TYPE alphabets_60004 DROP VALUE 'd' + +statement error could not remove enum value "c" as it is being used by table "using_alphabets_60004" +ALTER TYPE alphabets_60004 DROP VALUE 'c' + +statement error could not remove enum value "b" as it is being used by table "using_alphabets_60004" +ALTER TYPE alphabets_60004 DROP VALUE 'b' + +statement ok +TRUNCATE using_alphabets_60004 + +statement ok +ALTER TYPE alphabets_60004 DROP VALUE 'c' + +statement error could not remove enum value "a" as it is being used by "using_alphabets2_60004" +ALTER TYPE alphabets_60004 DROP VALUE 'a' + +statement error could not remove enum value "b" as it is being used by table "using_alphabets2_60004" +ALTER TYPE alphabets_60004 DROP VALUE 'b' + +statement ok +TRUNCATE using_alphabets2_60004 + +statement ok +ALTER TYPE alphabets_60004 DROP VALUE 'a' + +statement ok +ALTER TYPE alphabets_60004 DROP VALUE 'b' diff --git a/pkg/sql/type_change.go b/pkg/sql/type_change.go index 4ecc5a88864b..f04d96e9138b 100644 --- a/pkg/sql/type_change.go +++ b/pkg/sql/type_change.go @@ -720,6 +720,19 @@ func (t *typeSchemaChanger) cleanupEnumValues(ctx context.Context) error { return nil } +// convertToSQLStringRepresentation takes an array of bytes (the physical +// representation of an enum) and converts it into a string that can be used +// in a SQL predicate. +func convertToSQLStringRepresentation(bytes []byte) string { + var byteRep strings.Builder + byteRep.WriteString("b'") + for _, b := range bytes { + byteRep.WriteByte(b) + } + byteRep.WriteString("'") + return byteRep.String() +} + // canRemoveEnumValue returns an error if the enum value is in use and therefore // can't be removed. func (t *typeSchemaChanger) canRemoveEnumValue( @@ -729,19 +742,6 @@ func (t *typeSchemaChanger) canRemoveEnumValue( member *descpb.TypeDescriptor_EnumMember, descsCol *descs.Collection, ) error { - // convertToSQLStringRepresentation takes an array of bytes (the physical - // representation of an enum) and converts it into a string that can be used - // in a SQL predicate. - convertToSQLStringRepresentation := func(bytes []byte) string { - var byteRep strings.Builder - byteRep.WriteString("b'") - for _, b := range bytes { - byteRep.WriteByte(b) - } - byteRep.WriteString("'") - return byteRep.String() - } - for _, ID := range typeDesc.ReferencingDescriptorIDs { desc, err := descsCol.GetImmutableTableByID(ctx, txn, ID, tree.ObjectLookupFlags{}) if err != nil { @@ -814,8 +814,97 @@ func (t *typeSchemaChanger) canRemoveEnumValue( } } } - // We have ascertained that the value is not in use, and can therefore be - // safely removed. + + // Do validation for the array type now. + arrayTypeDesc, err := descsCol.GetImmutableTypeByID( + ctx, txn, typeDesc.ArrayTypeID, tree.ObjectLookupFlags{}) + if err != nil { + return err + } + + return t.canRemoveEnumValueFromArrayUsages(ctx, arrayTypeDesc, member, txn, descsCol) +} + +// canRemoveEnumValueFromArrayUsages returns an error if the enum member is used +// as a value by a table/view column which type resolves to a the given array +// type. +func (t *typeSchemaChanger) canRemoveEnumValueFromArrayUsages( + ctx context.Context, + arrayTypeDesc *typedesc.Immutable, + member *descpb.TypeDescriptor_EnumMember, + txn *kv.Txn, + descsCol *descs.Collection, +) error { + const validationErr = "could not validate removal of enum value %q" + for _, ID := range arrayTypeDesc.ReferencingDescriptorIDs { + desc, err := descsCol.GetImmutableTableByID(ctx, txn, ID, tree.ObjectLookupFlags{}) + if err != nil { + return errors.Wrapf(err, validationErr, member.LogicalRepresentation) + } + var whereClause strings.Builder + var unnestSelectClause strings.Builder + var query strings.Builder + + // Construct a query of the form: + // SELECT * FROM [ + // [SELECT unnest(c1), unnest(c2)... AS v1, v2... FROM + // [SELECT %d AS t] + // ]] + // WHERE v1 = 'enum_value' OR v2 = 'enum_value' OR ... LIMIT 1; + firstClause := true + unnestSelectClause.WriteString("SELECT ") + whereClause.WriteString("WHERE ") + for _, col := range desc.PublicColumns() { + if arrayTypeDesc.ID == typedesc.GetTypeDescID(col.GetType()) { + if !firstClause { + unnestSelectClause.WriteString(", ") + whereClause.WriteString("OR ") + } + unnestSelectClause.WriteString(fmt.Sprintf("unnest(%s) AS %s", col.GetName(), col.GetName())) + whereClause.WriteString(fmt.Sprintf("%s = %s", col.GetName(), convertToSQLStringRepresentation(member.PhysicalRepresentation))) + firstClause = false + } + } + // Unfortunately, we install a backreference to both the type descriptor and + // its array alias type regardless of the actual type of the table column. + // This means we may not actually construct a valid query after going + // through the columns, in which case there's no validation to do. + if firstClause { + continue + } + query.WriteString("SELECT * FROM [") + query.WriteString(unnestSelectClause.String()) + query.WriteString(" FROM [") + query.WriteString(fmt.Sprintf("SELECT * FROM [%d as t]]]", ID)) + query.WriteString(whereClause.String()) + query.WriteString(" LIMIT 1") + + _, dbDesc, err := descsCol.GetImmutableDatabaseByID( + ctx, txn, arrayTypeDesc.ParentID, tree.DatabaseLookupFlags{Required: true}) + if err != nil { + return errors.Wrapf(err, validationErr, member.LogicalRepresentation) + } + override := sessiondata.InternalExecutorOverride{ + User: security.RootUserName(), + Database: dbDesc.Name, + } + rows, err := t.execCfg.InternalExecutor.QueryRowEx( + ctx, + "count-array-type-value-usage", + txn, + override, + query.String(), + ) + if err != nil { + return errors.Wrapf(err, validationErr, member.LogicalRepresentation) + } + if len(rows) > 0 { + return errors.Newf("could not remove enum value %q as it is being used by table %q", + member.LogicalRepresentation, desc.GetName(), + ) + } + } + // None of the tables use the enum member in their rows. return nil }