Merge cockroachdb#79590 cockroachdb#79659

79590: sql/schemachanger/scrun: add validation in makeState r=ajwerner a=ajwerner This addresses a TODO and has a test. Release note: None 79659: sql: version gating pkey virtual column validation. r=fqazi a=fqazi Fixes: cockroachdb#79329 Previously, the schemachange workload on master was failing because 21.2 does not support virtual columns inside primary keys. So, the mixed version variant of the schemachange workload can fail when a primary key with virtual columns exists, specifically these tables become inaccessible from 21.2 nodes. To address, this patch introduces a version gate to prevent creating prevent creating primary indexes with virtual columns and updates the schemachange workload to detect when this error will be generated in mixed version workloads. Release note: None Co-authored-by: Andrew Werner <[email protected]> Co-authored-by: Chengxiong Ruan <[email protected]>
ajwerner · Apr 8, 2022 · d53a4c0 · d53a4c0
3 parents fb54ea7 + d7fb91f + 081e12d
commit d53a4c0
Show file tree

Hide file tree

Showing 18 changed files with 627 additions and 79 deletions.
diff --git a/pkg/BUILD.bazel b/pkg/BUILD.bazel
@@ -361,6 +361,7 @@ ALL_TESTS = [
     "//pkg/sql/schemachanger/scplan/internal/scgraph:scgraph_test",
     "//pkg/sql/schemachanger/scplan:scplan_test",
     "//pkg/sql/schemachanger/screl:screl_test",
+    "//pkg/sql/schemachanger/scrun:scrun_test",
     "//pkg/sql/schemachanger:schemachanger_test",
     "//pkg/sql/sem/builtins:builtins_test",
     "//pkg/sql/sem/tree/cast_test:cast_test_test",

diff --git a/pkg/ccl/schemachangerccl/testdata/end_to_end/drop_multiregion b/pkg/ccl/schemachangerccl/testdata/end_to_end/drop_multiregion
@@ -28,7 +28,8 @@ upsert descriptor #105
    type:
      arrayTypeId: 107
   +  declarativeSchemaChangerState:
-  +    authorization: {}
+  +    authorization:
+  +      userName: root
   +    jobId: "1"
      enumMembers:
      - logicalRepresentation: us-east1
@@ -46,7 +47,8 @@ upsert descriptor #107
        family: ArrayFamily
        oid: 100107
   +  declarativeSchemaChangerState:
-  +    authorization: {}
+  +    authorization:
+  +      userName: root
   +    jobId: "1"
      id: 107
      kind: ALIAS
@@ -62,7 +64,8 @@ upsert descriptor #108
      createAsOfTime:
        wallTime: "1"
   +  declarativeSchemaChangerState:
-  +    authorization: {}
+  +    authorization:
+  +      userName: root
   +    currentStatuses:
   +    - ABSENT
   +    - ABSENT
@@ -370,7 +373,8 @@ upsert descriptor #105
    type:
      arrayTypeId: 107
   -  declarativeSchemaChangerState:
-  -    authorization: {}
+  -    authorization:
+  -      userName: root
   -    jobId: "1"
      enumMembers:
      - logicalRepresentation: us-east1
@@ -384,7 +388,8 @@ upsert descriptor #107
        family: ArrayFamily
        oid: 100107
   -  declarativeSchemaChangerState:
-  -    authorization: {}
+  -    authorization:
+  -      userName: root
   -    jobId: "1"
      id: 107
      kind: ALIAS
@@ -398,7 +403,8 @@ upsert descriptor #108
      createAsOfTime:
        wallTime: "1"
   -  declarativeSchemaChangerState:
-  -    authorization: {}
+  -    authorization:
+  -      userName: root
   -    currentStatuses:
   -    - ABSENT
   -    - ABSENT
@@ -720,7 +726,8 @@ upsert descriptor #105
    type:
      arrayTypeId: 107
   +  declarativeSchemaChangerState:
-  +    authorization: {}
+  +    authorization:
+  +      userName: root
   +    jobId: "1"
      enumMembers:
      - logicalRepresentation: us-east1
@@ -738,7 +745,8 @@ upsert descriptor #109
      createAsOfTime:
        wallTime: "1"
   +  declarativeSchemaChangerState:
-  +    authorization: {}
+  +    authorization:
+  +      userName: root
   +    currentStatuses:
   +    - ABSENT
   +    - ABSENT
@@ -954,7 +962,8 @@ upsert descriptor #105
    type:
      arrayTypeId: 107
   -  declarativeSchemaChangerState:
-  -    authorization: {}
+  -    authorization:
+  -      userName: root
   -    jobId: "1"
      enumMembers:
      - logicalRepresentation: us-east1
@@ -968,7 +977,8 @@ upsert descriptor #109
      createAsOfTime:
        wallTime: "1"
   -  declarativeSchemaChangerState:
-  -    authorization: {}
+  -    authorization:
+  -      userName: root
   -    currentStatuses:
   -    - ABSENT
   -    - ABSENT
@@ -1192,7 +1202,8 @@ delete object namespace entry {104 106 _crdb_internal_region} -> 107
 upsert descriptor #104
    database:
   +  declarativeSchemaChangerState:
-  +    authorization: {}
+  +    authorization:
+  +      userName: root
   +    currentStatuses:
   +    - ABSENT
   +    - ABSENT
@@ -1306,7 +1317,8 @@ upsert descriptor #105
    type:
      arrayTypeId: 107
   +  declarativeSchemaChangerState:
-  +    authorization: {}
+  +    authorization:
+  +      userName: root
   +    currentStatuses:
   +    - ABSENT
   +    - ABSENT
@@ -1403,7 +1415,8 @@ upsert descriptor #105
 upsert descriptor #106
    schema:
   +  declarativeSchemaChangerState:
-  +    authorization: {}
+  +    authorization:
+  +      userName: root
   +    currentStatuses:
   +    - ABSENT
   +    - ABSENT
@@ -1510,7 +1523,8 @@ upsert descriptor #107
        family: ArrayFamily
        oid: 100107
   +  declarativeSchemaChangerState:
-  +    authorization: {}
+  +    authorization:
+  +      userName: root
   +    currentStatuses:
   +    - ABSENT
   +    - ABSENT

diff --git a/pkg/sql/alter_primary_key.go b/pkg/sql/alter_primary_key.go
@@ -14,6 +14,7 @@ import (
 	"context"
 	"time"
 
+	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/server/telemetry"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
@@ -134,6 +135,11 @@ func (p *planner) AlterPrimaryKey(
 		if col.IsNullable() {
 			return pgerror.Newf(pgcode.InvalidSchemaDefinition, "cannot use nullable column %q in primary key", col.GetName())
 		}
+		if !p.EvalContext().Settings.Version.IsActive(ctx, clusterversion.Start22_1) {
+			if col.IsVirtual() {
+				return pgerror.Newf(pgcode.FeatureNotSupported, "cannot use virtual column %q in primary key", col.GetName())
+			}
+		}
 	}
 
 	// Validate if the end result is the same as the current

diff --git a/pkg/sql/catalog/schemadesc/synthetic_schema_desc.go b/pkg/sql/catalog/schemadesc/synthetic_schema_desc.go
@@ -94,7 +94,8 @@ func (p synthetic) NewBuilder() catalog.DescriptorBuilder {
 func (p synthetic) GetReferencedDescIDs() (catalog.DescriptorIDSet, error) {
 	return catalog.DescriptorIDSet{}, nil
 }
-func (p synthetic) ValidateSelf(_ catalog.ValidationErrorAccumulator) {}
+func (p synthetic) ValidateSelf(_ catalog.ValidationErrorAccumulator) {
+}
 func (p synthetic) ValidateCrossReferences(
 	_ catalog.ValidationErrorAccumulator, _ catalog.ValidationDescGetter,
 ) {

diff --git a/pkg/sql/catalog/tabledesc/validate.go b/pkg/sql/catalog/tabledesc/validate.go
@@ -13,6 +13,7 @@ package tabledesc
 import (
 	"sort"
 
+	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
@@ -604,7 +605,7 @@ func (desc *wrapper) ValidateSelf(vea catalog.ValidationErrorAccumulator) {
 			desc.validateColumnFamilies(columnIDs),
 			desc.validateCheckConstraints(columnIDs),
 			desc.validateUniqueWithoutIndexConstraints(columnIDs),
-			desc.validateTableIndexes(columnNames),
+			desc.validateTableIndexes(columnNames, vea),
 			desc.validatePartitioning(),
 		}
 		hasErrs := false
@@ -1043,7 +1044,9 @@ func (desc *wrapper) validateUniqueWithoutIndexConstraints(
 // IDs are unique, and the family of the primary key is 0. This does not check
 // if indexes are unique (i.e. same set of columns, direction, and uniqueness)
 // as there are practical uses for them.
-func (desc *wrapper) validateTableIndexes(columnNames map[string]descpb.ColumnID) error {
+func (desc *wrapper) validateTableIndexes(
+	columnNames map[string]descpb.ColumnID, vea catalog.ValidationErrorAccumulator,
+) error {
 	if len(desc.PrimaryIndex.KeyColumnIDs) == 0 {
 		return ErrMissingPrimaryKey
 	}
@@ -1053,6 +1056,15 @@ func (desc *wrapper) validateTableIndexes(columnNames map[string]descpb.ColumnID
 		columnsByID[col.GetID()] = col
 	}
 
+	if !vea.IsActive(clusterversion.Start22_1) {
+		// Verify that the primary index columns are not virtual.
+		for _, pkID := range desc.PrimaryIndex.KeyColumnIDs {
+			if col := columnsByID[pkID]; col != nil && col.IsVirtual() {
+				return errors.Newf("primary index column %q cannot be virtual", col.GetName())
+			}
+		}
+	}
+
 	indexNames := map[string]struct{}{}
 	indexIDs := map[descpb.IndexID]string{}
 	for _, idx := range desc.NonDropIndexes() {
@@ -1204,6 +1216,12 @@ func (desc *wrapper) validateTableIndexes(columnNames map[string]descpb.ColumnID
 			}
 		}
 		for _, colID := range idx.IndexDesc().KeySuffixColumnIDs {
+			if !vea.IsActive(clusterversion.Start22_1) {
+				if col := columnsByID[colID]; col != nil && col.IsVirtual() {
+					return errors.Newf("index %q cannot store virtual column %d", idx.GetName(), colID)
+				}
+			}
+
 			if _, ok := columnsByID[colID]; !ok {
 				return errors.Newf("column %d does not exist in table %s", colID, desc.Name)
 			}

diff --git a/pkg/sql/catalog/tabledesc/validate_test.go b/pkg/sql/catalog/tabledesc/validate_test.go
@@ -1901,6 +1901,153 @@ func TestValidateTableDesc(t *testing.T) {
 	}
 }
 
+func TestPrimaryKeyCannotBeVirtualBefore22_1(t *testing.T) {
+	computedExpr := "1 + 1"
+	testData := []struct {
+		err  string
+		desc descpb.TableDescriptor
+	}{
+		{
+			err: `primary index column "c3" cannot be virtual`,
+			desc: descpb.TableDescriptor{
+				ID:            2,
+				ParentID:      1,
+				Name:          "foo",
+				FormatVersion: descpb.InterleavedFormatVersion,
+				Columns: []descpb.ColumnDescriptor{
+					{ID: 1, Name: "c1"},
+					{ID: 2, Name: "c2"},
+					{ID: 3, Name: "c3", ComputeExpr: &computedExpr, Virtual: true},
+				},
+				PrimaryIndex: descpb.IndexDescriptor{
+					ID:                  1,
+					Name:                "primary",
+					Unique:              true,
+					KeyColumnIDs:        []descpb.ColumnID{1, 3},
+					KeyColumnNames:      []string{"c1", "c3"},
+					KeyColumnDirections: []descpb.IndexDescriptor_Direction{descpb.IndexDescriptor_ASC, descpb.IndexDescriptor_ASC},
+					Version:             descpb.LatestIndexDescriptorVersion,
+					EncodingType:        descpb.PrimaryIndexEncoding,
+				},
+				Indexes: []descpb.IndexDescriptor{
+					{ID: 2, Name: "sec", KeyColumnIDs: []descpb.ColumnID{2},
+						KeyColumnNames:      []string{"c2"},
+						KeyColumnDirections: []descpb.IndexDescriptor_Direction{descpb.IndexDescriptor_ASC},
+						KeySuffixColumnIDs:  []descpb.ColumnID{1, 3},
+					},
+				},
+				Families: []descpb.ColumnFamilyDescriptor{
+					{ID: 0, Name: "primary",
+						ColumnIDs:   []descpb.ColumnID{1, 2},
+						ColumnNames: []string{"c1", "c2"},
+					},
+				},
+				Mutations:    []descpb.DescriptorMutation{},
+				NextColumnID: 4,
+				NextFamilyID: 1,
+				NextIndexID:  5,
+				Privileges:   catpb.NewBasePrivilegeDescriptor(security.AdminRoleName()),
+			},
+		},
+		{
+			err: `index "sec" cannot store virtual column 3`,
+			desc: descpb.TableDescriptor{
+				ID:            2,
+				ParentID:      1,
+				Name:          "foo",
+				FormatVersion: descpb.InterleavedFormatVersion,
+				Columns: []descpb.ColumnDescriptor{
+					{ID: 1, Name: "c1"},
+					{ID: 2, Name: "c2"},
+					{ID: 3, Name: "c3", ComputeExpr: &computedExpr, Virtual: true},
+				},
+				PrimaryIndex: descpb.IndexDescriptor{
+					ID:                  1,
+					Name:                "primary",
+					Unique:              true,
+					KeyColumnIDs:        []descpb.ColumnID{1},
+					KeyColumnNames:      []string{"c1"},
+					KeyColumnDirections: []descpb.IndexDescriptor_Direction{descpb.IndexDescriptor_ASC},
+					Version:             descpb.LatestIndexDescriptorVersion,
+					EncodingType:        descpb.PrimaryIndexEncoding,
+				},
+				Indexes: []descpb.IndexDescriptor{
+					{ID: 2, Name: "sec", KeyColumnIDs: []descpb.ColumnID{2},
+						KeyColumnNames:      []string{"c2"},
+						KeyColumnDirections: []descpb.IndexDescriptor_Direction{descpb.IndexDescriptor_ASC},
+						KeySuffixColumnIDs:  []descpb.ColumnID{1, 3},
+					},
+				},
+				Families: []descpb.ColumnFamilyDescriptor{
+					{ID: 0, Name: "primary",
+						ColumnIDs:   []descpb.ColumnID{1, 2},
+						ColumnNames: []string{"c1", "c2"},
+					},
+				},
+				Mutations: []descpb.DescriptorMutation{
+					{
+						Descriptor_: &descpb.DescriptorMutation_Index{
+							Index: &descpb.IndexDescriptor{
+								ID:                  3,
+								Name:                "new_primary_key",
+								Unique:              true,
+								KeyColumnIDs:        []descpb.ColumnID{1, 3},
+								KeyColumnNames:      []string{"c1", "c3"},
+								KeyColumnDirections: []descpb.IndexDescriptor_Direction{descpb.IndexDescriptor_ASC, descpb.IndexDescriptor_ASC},
+								Version:             descpb.LatestIndexDescriptorVersion,
+								EncodingType:        descpb.PrimaryIndexEncoding,
+							},
+						},
+						Direction: descpb.DescriptorMutation_ADD,
+						State:     descpb.DescriptorMutation_DELETE_ONLY,
+					},
+					{
+						Descriptor_: &descpb.DescriptorMutation_Index{
+							Index: &descpb.IndexDescriptor{
+								ID: 4, Name: "new_sec", KeyColumnIDs: []descpb.ColumnID{2},
+								KeyColumnNames:      []string{"c2"},
+								KeyColumnDirections: []descpb.IndexDescriptor_Direction{descpb.IndexDescriptor_ASC},
+								KeySuffixColumnIDs:  []descpb.ColumnID{1, 3},
+							},
+						},
+						Direction: descpb.DescriptorMutation_ADD,
+						State:     descpb.DescriptorMutation_DELETE_ONLY,
+					},
+					{
+						Descriptor_: &descpb.DescriptorMutation_PrimaryKeySwap{
+							PrimaryKeySwap: &descpb.PrimaryKeySwap{
+								OldPrimaryIndexId: 1,
+								NewPrimaryIndexId: 3,
+								NewIndexes:        []descpb.IndexID{4},
+								OldIndexes:        []descpb.IndexID{2},
+							},
+						},
+						Direction: descpb.DescriptorMutation_ADD,
+						State:     descpb.DescriptorMutation_DELETE_ONLY,
+					},
+				},
+				NextColumnID: 4,
+				NextFamilyID: 1,
+				NextIndexID:  5,
+				Privileges:   catpb.NewBasePrivilegeDescriptor(security.AdminRoleName()),
+			},
+		},
+	}
+	for i, d := range testData {
+		t.Run(d.err, func(t *testing.T) {
+			d.desc.Privileges = catpb.NewBasePrivilegeDescriptor(security.RootUserName())
+			desc := NewBuilder(&d.desc).BuildImmutableTable()
+			expectedErr := fmt.Sprintf("%s %q (%d): %s", desc.DescriptorType(), desc.GetName(), desc.GetID(), d.err)
+			err := validate.Self(clusterversion.ClusterVersion{Version: clusterversion.ByKey(clusterversion.V21_2)}, desc)
+			if err == nil {
+				t.Errorf("%d: expected \"%s\", but found success: %+v", i, expectedErr, d.desc)
+			} else if expectedErr != err.Error() {
+				t.Errorf("%d: expected \"%s\", but found \"%+v\"", i, expectedErr, err)
+			}
+		})
+	}
+}
+
 func TestValidateCrossTableReferences(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	ctx := context.Background()

diff --git a/pkg/sql/catalog/typedesc/table_implicit_record_type.go b/pkg/sql/catalog/typedesc/table_implicit_record_type.go
@@ -205,7 +205,8 @@ func (v TableImplicitRecordType) GetReferencedDescIDs() (catalog.DescriptorIDSet
 }
 
 // ValidateSelf implements the Descriptor interface.
-func (v TableImplicitRecordType) ValidateSelf(_ catalog.ValidationErrorAccumulator) {}
+func (v TableImplicitRecordType) ValidateSelf(_ catalog.ValidationErrorAccumulator) {
+}
 
 // ValidateCrossReferences implements the Descriptor interface.
 func (v TableImplicitRecordType) ValidateCrossReferences(