From d7fb91fe8fce994fbfc5984ef47408da3ca10253 Mon Sep 17 00:00:00 2001 From: Andrew Werner Date: Thu, 7 Apr 2022 12:15:11 -0400 Subject: [PATCH 1/2] sql/schemachanger/scrun: add validation in makeState This addresses a TODO and has a test. It also fixes a bug in the testing infrastructure whereby we were using the wrong protocol buffer message type to deserialize session data. Release note: None --- pkg/BUILD.bazel | 1 + .../testdata/end_to_end/drop_multiregion | 42 +++-- .../scdeps/sctestdeps/database_state.go | 14 +- pkg/sql/schemachanger/scrun/BUILD.bazel | 20 ++- .../schemachanger/scrun/make_state_test.go | 162 ++++++++++++++++++ pkg/sql/schemachanger/scrun/scrun.go | 82 +++++++-- .../testdata/alter_table_add_column | 10 +- pkg/sql/schemachanger/testdata/drop | 96 +++++++---- pkg/sql/schemachanger/testdata/index | 8 +- 9 files changed, 360 insertions(+), 75 deletions(-) create mode 100644 pkg/sql/schemachanger/scrun/make_state_test.go diff --git a/pkg/BUILD.bazel b/pkg/BUILD.bazel index f3b3dd24e70c..f517bea2568b 100644 --- a/pkg/BUILD.bazel +++ b/pkg/BUILD.bazel @@ -358,6 +358,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 index 5c67ded00c47..0d0ffae39fe8 100644 --- 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/schemachanger/scdeps/sctestdeps/database_state.go b/pkg/sql/schemachanger/scdeps/sctestdeps/database_state.go index ee2f326950f7..a53b07067d41 100644 --- a/pkg/sql/schemachanger/scdeps/sctestdeps/database_state.go +++ b/pkg/sql/schemachanger/scdeps/sctestdeps/database_state.go @@ -144,7 +144,7 @@ func ReadCurrentDatabaseFromDB(t *testing.T, tdb *sqlutils.SQLRunner) (db string // allows the caller to modify it with the passed function. func ReadSessionDataFromDB( t *testing.T, tdb *sqlutils.SQLRunner, override func(sd *sessiondata.SessionData), -) (sd sessiondata.SessionData) { +) sessiondata.SessionData { hexSessionData := tdb.QueryStr(t, `SELECT encode(crdb_internal.serialize_session(), 'hex')`) if len(hexSessionData) == 0 { t.Fatal("Empty session data query results.") @@ -153,16 +153,16 @@ func ReadSessionDataFromDB( if err != nil { t.Fatal(err) } - sessionDataProto := sessiondatapb.SessionData{} - err = protoutil.Unmarshal(sessionDataBytes, &sessionDataProto) + var m sessiondatapb.MigratableSession + err = protoutil.Unmarshal(sessionDataBytes, &m) if err != nil { t.Fatal(err) } - sessionData, err := sessiondata.UnmarshalNonLocal(sessionDataProto) + sd, err := sessiondata.UnmarshalNonLocal(m.SessionData) if err != nil { t.Fatal(err) } - sd = *sessionData - override(&sd) - return sd + sd.SessionData = m.SessionData + override(sd) + return *sd } diff --git a/pkg/sql/schemachanger/scrun/BUILD.bazel b/pkg/sql/schemachanger/scrun/BUILD.bazel index 5d4dd5e5d36d..e537191d4500 100644 --- a/pkg/sql/schemachanger/scrun/BUILD.bazel +++ b/pkg/sql/schemachanger/scrun/BUILD.bazel @@ -1,4 +1,4 @@ -load("@io_bazel_rules_go//go:def.bzl", "go_library") +load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "scrun", @@ -14,6 +14,7 @@ go_library( "//pkg/jobs/jobspb", "//pkg/roachpb", "//pkg/settings/cluster", + "//pkg/sql/catalog", "//pkg/sql/catalog/descpb", "//pkg/sql/schemachanger/scexec", "//pkg/sql/schemachanger/scop", @@ -24,3 +25,20 @@ go_library( "@com_github_cockroachdb_errors//:errors", ], ) + +go_test( + name = "scrun_test", + size = "small", + srcs = ["make_state_test.go"], + embed = [":scrun"], + deps = [ + "//pkg/jobs/jobspb", + "//pkg/sql/catalog", + "//pkg/sql/catalog/descpb", + "//pkg/sql/catalog/tabledesc", + "//pkg/sql/schemachanger/scexec", + "//pkg/sql/schemachanger/scpb", + "//pkg/util/leaktest", + "@com_github_stretchr_testify//require", + ], +) diff --git a/pkg/sql/schemachanger/scrun/make_state_test.go b/pkg/sql/schemachanger/scrun/make_state_test.go new file mode 100644 index 000000000000..c20132d33f40 --- /dev/null +++ b/pkg/sql/schemachanger/scrun/make_state_test.go @@ -0,0 +1,162 @@ +// Copyright 2022 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package scrun + +import ( + "context" + "testing" + + "github.com/cockroachdb/cockroach/pkg/jobs/jobspb" + "github.com/cockroachdb/cockroach/pkg/sql/catalog" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" + "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scexec" + "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb" + "github.com/cockroachdb/cockroach/pkg/util/leaktest" + "github.com/stretchr/testify/require" +) + +// fakeCatalog is a fake implementation of scexec.Catalog for testing makeState. +type fakeCatalog struct { + descs map[descpb.ID]catalog.Descriptor + scexec.Catalog +} + +func (fc fakeCatalog) MustReadImmutableDescriptors( + ctx context.Context, ids ...descpb.ID, +) ([]catalog.Descriptor, error) { + ret := make([]catalog.Descriptor, len(ids)) + for i, id := range ids { + d, ok := fc.descs[id] + if !ok { + panic("boom") + } + ret[i] = d + } + return ret, nil +} + +// TestMakeState tests some validation checking in the makeState function. +func TestMakeState(t *testing.T) { + defer leaktest.AfterTest(t)() + + ctx := context.Background() + + for _, tc := range []struct { + name string + ids []descpb.ID + rollback bool + jobID jobspb.JobID + descriptors []catalog.Descriptor + expErr string + }{ + { + name: "missing job ID", + jobID: 1, + ids: []descpb.ID{2}, + expErr: `descriptor "foo" \(2\): missing job ID in schema changer state`, + descriptors: []catalog.Descriptor{ + tabledesc.NewBuilder(&descpb.TableDescriptor{ + Name: "foo", + ID: 2, + DeclarativeSchemaChangerState: &scpb.DescriptorState{ + Authorization: scpb.Authorization{ + UserName: "user1", + AppName: "app1", + }, + }, + }).BuildImmutable(), + }, + }, + { + name: "mismatched job ID", + jobID: 1, + ids: []descpb.ID{2}, + expErr: `descriptor "foo" \(2\): job ID mismatch: expected 1, got 2`, + descriptors: []catalog.Descriptor{ + tabledesc.NewBuilder(&descpb.TableDescriptor{ + Name: "foo", + ID: 2, + DeclarativeSchemaChangerState: &scpb.DescriptorState{ + JobID: 2, + Authorization: scpb.Authorization{ + UserName: "user1", + AppName: "app1", + }, + }, + }).BuildImmutable(), + }, + }, + { + name: "missing authorization", + jobID: 1, + ids: []descpb.ID{2}, + expErr: `descriptor "foo" \(2\): missing authorization in schema changer state`, + descriptors: []catalog.Descriptor{ + tabledesc.NewBuilder(&descpb.TableDescriptor{ + Name: "foo", + ID: 2, + DeclarativeSchemaChangerState: &scpb.DescriptorState{ + JobID: 1, + }, + }).BuildImmutable(), + }, + }, + { + name: "mismatched authorization", + jobID: 1, + ids: []descpb.ID{2, 3}, + expErr: `descriptor "bar" \(3\): authorization mismatch: expected {user1 app1}, got {user2 app1}`, + descriptors: []catalog.Descriptor{ + tabledesc.NewBuilder(&descpb.TableDescriptor{ + Name: "foo", + ID: 2, + DeclarativeSchemaChangerState: &scpb.DescriptorState{ + JobID: 1, + Authorization: scpb.Authorization{ + UserName: "user1", + AppName: "app1", + }, + }, + }).BuildImmutable(), + tabledesc.NewBuilder(&descpb.TableDescriptor{ + Name: "bar", + ID: 3, + DeclarativeSchemaChangerState: &scpb.DescriptorState{ + JobID: 1, + Authorization: scpb.Authorization{ + UserName: "user2", + AppName: "app1", + }, + }, + }).BuildImmutable(), + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + cat := fakeCatalog{ + descs: map[descpb.ID]catalog.Descriptor{}, + } + for _, d := range tc.descriptors { + cat.descs[d.GetID()] = d + } + _, err := makeState(ctx, 1, tc.ids, true, func( + ctx context.Context, f func(context.Context, scexec.Catalog) error) error { + return f(ctx, cat) + }) + if tc.expErr == "" { + require.NoError(t, err) + } else { + require.Regexp(t, tc.expErr, err) + } + }) + } +} diff --git a/pkg/sql/schemachanger/scrun/scrun.go b/pkg/sql/schemachanger/scrun/scrun.go index 725b0923dea6..ebe76bc39be8 100644 --- a/pkg/sql/schemachanger/scrun/scrun.go +++ b/pkg/sql/schemachanger/scrun/scrun.go @@ -18,6 +18,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/jobs/jobspb" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/settings/cluster" + "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scexec" "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scop" @@ -89,7 +90,15 @@ func RunSchemaChangesInJob( descriptorIDs []descpb.ID, rollback bool, ) error { - state, err := makeState(ctx, deps, descriptorIDs, rollback) + state, err := makeState(ctx, jobID, descriptorIDs, rollback, func( + ctx context.Context, f catalogFunc, + ) error { + return deps.WithTxnInJob(ctx, func( + ctx context.Context, txnDeps scexec.Dependencies, + ) error { + return f(ctx, txnDeps.Catalog()) + }) + }) if err != nil { return errors.Wrapf(err, "failed to construct state for job %d", jobID) } @@ -170,14 +179,65 @@ func executeStage( return nil } +type ( + catalogFunc = func(context.Context, scexec.Catalog) error + withCatalogFunc = func(context.Context, catalogFunc) error +) + func makeState( - ctx context.Context, deps JobRunDependencies, descriptorIDs []descpb.ID, rollback bool, + ctx context.Context, + jobID jobspb.JobID, + descriptorIDs []descpb.ID, + rollback bool, + withCatalog withCatalogFunc, ) (scpb.CurrentState, error) { + descError := func(desc catalog.Descriptor, err error) error { + return errors.Wrapf(err, "descriptor %q (%d)", desc.GetName(), desc.GetID()) + } + validateJobID := func(fromDesc jobspb.JobID) error { + switch { + case fromDesc == jobspb.InvalidJobID: + return errors.New("missing job ID in schema changer state") + case fromDesc != jobID: + return errors.Errorf("job ID mismatch: expected %d, got %d", + jobID, fromDesc) + default: + return nil + } + } + var authorization scpb.Authorization + validateAuthorization := func(fromDesc scpb.Authorization) error { + switch { + case fromDesc == (scpb.Authorization{}): + return errors.New("missing authorization in schema changer state") + case authorization == (scpb.Authorization{}): + authorization = fromDesc + case authorization != fromDesc: + return errors.Errorf("authorization mismatch: expected %v, got %v", + authorization, fromDesc) + } + return nil + } var descriptorStates []*scpb.DescriptorState - if err := deps.WithTxnInJob(ctx, func(ctx context.Context, txnDeps scexec.Dependencies) error { - descriptorStates = nil - // Reset for restarts. - descs, err := txnDeps.Catalog().MustReadImmutableDescriptors(ctx, descriptorIDs...) + addDescriptorState := func(desc catalog.Descriptor) error { + cs := desc.GetDeclarativeSchemaChangerState() + if cs == nil { + return errors.New("missing schema changer state") + } + if err := validateJobID(cs.JobID); err != nil { + return err + } + if err := validateAuthorization(cs.Authorization); err != nil { + return err + } + descriptorStates = append(descriptorStates, cs) + return nil + } + if err := withCatalog(ctx, func( + ctx context.Context, cat scexec.Catalog, + ) error { + descriptorStates = nil // reset for restarts + descs, err := cat.MustReadImmutableDescriptors(ctx, descriptorIDs...) if err != nil { // TODO(ajwerner): It seems possible that a descriptor could be deleted // and the schema change is in a happy place. Ideally we'd enforce that @@ -186,15 +246,9 @@ func makeState( return err } for _, desc := range descs { - // TODO(ajwerner): Verify that the job ID matches on all of the - // descriptors. Also verify that the Authorization matches. - cs := desc.GetDeclarativeSchemaChangerState() - if cs == nil { - return errors.Errorf( - "descriptor %q (%d) does not contain schema changer state", desc.GetName(), desc.GetID(), - ) + if err := addDescriptorState(desc); err != nil { + return descError(desc, err) } - descriptorStates = append(descriptorStates, cs) } return nil }); err != nil { diff --git a/pkg/sql/schemachanger/testdata/alter_table_add_column b/pkg/sql/schemachanger/testdata/alter_table_add_column index 161ec2639edd..2f9352a256de 100644 --- a/pkg/sql/schemachanger/testdata/alter_table_add_column +++ b/pkg/sql/schemachanger/testdata/alter_table_add_column @@ -23,7 +23,8 @@ upsert descriptor #106 createAsOfTime: wallTime: "1" + declarativeSchemaChangerState: - + authorization: {} + + authorization: + + userName: root + currentStatuses: + - PUBLIC + - PUBLIC @@ -272,7 +273,7 @@ upsert descriptor #106 createAsOfTime: wallTime: "1" ... - authorization: {} + userName: root currentStatuses: + - VALIDATED + - ABSENT @@ -377,7 +378,7 @@ begin transaction #7 ## PostCommitNonRevertiblePhase stage 1 of 2 with 3 MutationType ops upsert descriptor #106 ... - authorization: {} + userName: root currentStatuses: - - VALIDATED + - DELETE_ONLY @@ -404,7 +405,8 @@ upsert descriptor #106 createAsOfTime: wallTime: "1" - declarativeSchemaChangerState: - - authorization: {} + - authorization: + - userName: root - currentStatuses: - - DELETE_ONLY - - ABSENT diff --git a/pkg/sql/schemachanger/testdata/drop b/pkg/sql/schemachanger/testdata/drop index d4a37a65a2c1..92a62b5592d1 100644 --- a/pkg/sql/schemachanger/testdata/drop +++ b/pkg/sql/schemachanger/testdata/drop @@ -23,7 +23,8 @@ delete schema namespace entry {104 0 sc} -> 106 upsert descriptor #104 database: + declarativeSchemaChangerState: - + authorization: {} + + authorization: + + userName: root + jobId: "1" id: 104 modificationTime: {} @@ -37,7 +38,8 @@ upsert descriptor #104 upsert descriptor #106 schema: + declarativeSchemaChangerState: - + authorization: {} + + authorization: + + userName: root + currentStatuses: + - ABSENT + - ABSENT @@ -140,7 +142,8 @@ begin transaction #3 upsert descriptor #104 database: - declarativeSchemaChangerState: - - authorization: {} + - authorization: + - userName: root - jobId: "1" id: 104 modificationTime: {} @@ -183,7 +186,8 @@ upsert descriptor #108 createAsOfTime: wallTime: "1" + declarativeSchemaChangerState: - + authorization: {} + + authorization: + + userName: root + currentStatuses: + - ABSENT + - ABSENT @@ -500,7 +504,8 @@ upsert descriptor #108 createAsOfTime: wallTime: "1" - declarativeSchemaChangerState: - - authorization: {} + - authorization: + - userName: root - currentStatuses: - - ABSENT - - ABSENT @@ -824,7 +829,8 @@ delete object namespace entry {104 107 _e} -> 110 upsert descriptor #104 database: + declarativeSchemaChangerState: - + authorization: {} + + authorization: + + userName: root + jobId: "1" id: 104 modificationTime: {} @@ -838,7 +844,8 @@ upsert descriptor #104 upsert descriptor #107 schema: + declarativeSchemaChangerState: - + authorization: {} + + authorization: + + userName: root + currentStatuses: + - ABSENT + - ABSENT @@ -932,7 +939,8 @@ upsert descriptor #109 type: arrayTypeId: 110 + declarativeSchemaChangerState: - + authorization: {} + + authorization: + + userName: root + currentStatuses: + - ABSENT + - ABSENT @@ -1030,7 +1038,8 @@ upsert descriptor #110 family: ArrayFamily oid: 100110 + declarativeSchemaChangerState: - + authorization: {} + + authorization: + + userName: root + currentStatuses: + - ABSENT + - ABSENT @@ -1148,7 +1157,8 @@ begin transaction #3 upsert descriptor #104 database: - declarativeSchemaChangerState: - - authorization: {} + - authorization: + - userName: root - jobId: "1" id: 104 modificationTime: {} @@ -1181,7 +1191,8 @@ delete schema namespace entry {104 0 public} -> 105 upsert descriptor #104 database: + declarativeSchemaChangerState: - + authorization: {} + + authorization: + + userName: root + currentStatuses: + - ABSENT + - ABSENT @@ -1284,7 +1295,8 @@ upsert descriptor #104 upsert descriptor #105 schema: + declarativeSchemaChangerState: - + authorization: {} + + authorization: + + userName: root + currentStatuses: + - ABSENT + - ABSENT @@ -1467,7 +1479,8 @@ delete object namespace entry {111 113 v5} -> 125 upsert descriptor #111 database: + declarativeSchemaChangerState: - + authorization: {} + + authorization: + + userName: root + currentStatuses: + - ABSENT + - ABSENT @@ -1570,7 +1583,8 @@ upsert descriptor #111 upsert descriptor #112 schema: + declarativeSchemaChangerState: - + authorization: {} + + authorization: + + userName: root + currentStatuses: + - ABSENT + - ABSENT @@ -1675,7 +1689,8 @@ upsert descriptor #112 upsert descriptor #113 schema: + declarativeSchemaChangerState: - + authorization: {} + + authorization: + + userName: root + currentStatuses: + - ABSENT + - ABSENT @@ -1770,7 +1785,8 @@ upsert descriptor #114 createAsOfTime: wallTime: "1" + declarativeSchemaChangerState: - + authorization: {} + + authorization: + + userName: root + currentStatuses: + - ABSENT + - ABSENT @@ -1867,7 +1883,8 @@ upsert descriptor #115 createAsOfTime: wallTime: "1" + declarativeSchemaChangerState: - + authorization: {} + + authorization: + + userName: root + currentStatuses: + - ABSENT + - ABSENT @@ -1964,7 +1981,8 @@ upsert descriptor #116 createAsOfTime: wallTime: "1" + declarativeSchemaChangerState: - + authorization: {} + + authorization: + + userName: root + currentStatuses: + - ABSENT + - ABSENT @@ -2278,7 +2296,8 @@ upsert descriptor #117 createAsOfTime: wallTime: "1" + declarativeSchemaChangerState: - + authorization: {} + + authorization: + + userName: root + currentStatuses: + - ABSENT + - ABSENT @@ -2588,7 +2607,8 @@ upsert descriptor #118 createAsOfTime: wallTime: "1" + declarativeSchemaChangerState: - + authorization: {} + + authorization: + + userName: root + currentStatuses: + - ABSENT + - ABSENT @@ -2868,7 +2888,8 @@ upsert descriptor #119 createAsOfTime: wallTime: "1" + declarativeSchemaChangerState: - + authorization: {} + + authorization: + + userName: root + currentStatuses: + - ABSENT + - ABSENT @@ -3018,7 +3039,8 @@ upsert descriptor #120 createAsOfTime: wallTime: "1" + declarativeSchemaChangerState: - + authorization: {} + + authorization: + + userName: root + currentStatuses: + - ABSENT + - ABSENT @@ -3218,7 +3240,8 @@ upsert descriptor #121 createAsOfTime: wallTime: "1" + declarativeSchemaChangerState: - + authorization: {} + + authorization: + + userName: root + currentStatuses: + - ABSENT + - ABSENT @@ -3419,7 +3442,8 @@ upsert descriptor #122 createAsOfTime: wallTime: "1" + declarativeSchemaChangerState: - + authorization: {} + + authorization: + + userName: root + currentStatuses: + - ABSENT + - ABSENT @@ -3618,7 +3642,8 @@ upsert descriptor #123 type: arrayTypeId: 124 + declarativeSchemaChangerState: - + authorization: {} + + authorization: + + userName: root + currentStatuses: + - ABSENT + - ABSENT @@ -3716,7 +3741,8 @@ upsert descriptor #124 family: ArrayFamily oid: 100124 + declarativeSchemaChangerState: - + authorization: {} + + authorization: + + userName: root + currentStatuses: + - ABSENT + - ABSENT @@ -3826,7 +3852,8 @@ upsert descriptor #125 createAsOfTime: wallTime: "1" + declarativeSchemaChangerState: - + authorization: {} + + authorization: + + userName: root + currentStatuses: + - ABSENT + - ABSENT @@ -4091,7 +4118,8 @@ upsert descriptor #114 createAsOfTime: wallTime: "1" - declarativeSchemaChangerState: - - authorization: {} + - authorization: + - userName: root - currentStatuses: - - ABSENT - - ABSENT @@ -4186,7 +4214,8 @@ upsert descriptor #115 createAsOfTime: wallTime: "1" - declarativeSchemaChangerState: - - authorization: {} + - authorization: + - userName: root - currentStatuses: - - ABSENT - - ABSENT @@ -4281,7 +4310,8 @@ upsert descriptor #116 createAsOfTime: wallTime: "1" - declarativeSchemaChangerState: - - authorization: {} + - authorization: + - userName: root - currentStatuses: - - ABSENT - - ABSENT @@ -4588,7 +4618,8 @@ upsert descriptor #117 createAsOfTime: wallTime: "1" - declarativeSchemaChangerState: - - authorization: {} + - authorization: + - userName: root - currentStatuses: - - ABSENT - - ABSENT @@ -4895,7 +4926,8 @@ upsert descriptor #118 createAsOfTime: wallTime: "1" - declarativeSchemaChangerState: - - authorization: {} + - authorization: + - userName: root - currentStatuses: - - ABSENT - - ABSENT diff --git a/pkg/sql/schemachanger/testdata/index b/pkg/sql/schemachanger/testdata/index index 631343180667..527865fdc4d6 100644 --- a/pkg/sql/schemachanger/testdata/index +++ b/pkg/sql/schemachanger/testdata/index @@ -18,7 +18,8 @@ upsert descriptor #104 createAsOfTime: wallTime: "1" + declarativeSchemaChangerState: - + authorization: {} + + authorization: + + userName: root + currentStatuses: + - DELETE_ONLY + - ABSENT @@ -112,7 +113,7 @@ begin transaction #3 ## PostCommitPhase stage 1 of 4 with 3 MutationType ops upsert descriptor #104 ... - authorization: {} + userName: root currentStatuses: - - DELETE_ONLY + - WRITE_ONLY @@ -147,7 +148,8 @@ upsert descriptor #104 createAsOfTime: wallTime: "1" - declarativeSchemaChangerState: - - authorization: {} + - authorization: + - userName: root - currentStatuses: - - WRITE_ONLY - - ABSENT From 081e12dbe312262d16e52ded6cfd5f7f504f6430 Mon Sep 17 00:00:00 2001 From: Chengxiong Ruan Date: Mon, 4 Apr 2022 12:11:29 -0400 Subject: [PATCH 2/2] sql: version gating pkey virtual column validation. Fixes: #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 --- pkg/sql/alter_primary_key.go | 6 + .../schemadesc/synthetic_schema_desc.go | 3 +- pkg/sql/catalog/tabledesc/validate.go | 22 ++- pkg/sql/catalog/tabledesc/validate_test.go | 147 ++++++++++++++++++ .../typedesc/table_implicit_record_type.go | 3 +- pkg/sql/create_table.go | 10 ++ pkg/sql/schema_changer_test.go | 31 ++++ pkg/workload/schemachange/BUILD.bazel | 2 + .../schemachange/operation_generator.go | 47 ++++++ 9 files changed, 267 insertions(+), 4 deletions(-) diff --git a/pkg/sql/alter_primary_key.go b/pkg/sql/alter_primary_key.go index 795e0e1cc867..9292fc944d27 100644 --- 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 index 850636010463..5a77fceb6ab8 100644 --- 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 index 824768fe7c87..f501ab12cdb1 100644 --- 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 index 36b7a1b307a6..ce6f98f04ac1 100644 --- 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 index 2b3b6186f778..38b63aac23af 100644 --- 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( diff --git a/pkg/sql/create_table.go b/pkg/sql/create_table.go index c746065d6c6f..65e09d7eace5 100644 --- a/pkg/sql/create_table.go +++ b/pkg/sql/create_table.go @@ -1547,6 +1547,7 @@ func NewTableDesc( if err != nil { return nil, err } + primaryIndexColumnSet[shardCol.GetName()] = struct{}{} checkConstraint, err := makeShardCheckConstraintDef(int(buckets), shardCol) if err != nil { return nil, err @@ -1598,6 +1599,7 @@ func NewTableDesc( if err := desc.AddPrimaryIndex(*implicitColumnDefIdx.idx); err != nil { return nil, err } + primaryIndexColumnSet[string(implicitColumnDefIdx.def.Name)] = struct{}{} } else { // If it is a non-primary index that is implicitly created, ensure // partitioning for PARTITION ALL BY. @@ -1975,6 +1977,14 @@ func NewTableDesc( for i := range desc.Columns { if _, ok := primaryIndexColumnSet[desc.Columns[i].Name]; ok { + if !st.Version.IsActive(ctx, clusterversion.Start22_1) { + if desc.Columns[i].Virtual { + return nil, pgerror.Newf( + pgcode.FeatureNotSupported, + "cannot use virtual column %q in primary key", desc.Columns[i].Name, + ) + } + } desc.Columns[i].Nullable = false } } diff --git a/pkg/sql/schema_changer_test.go b/pkg/sql/schema_changer_test.go index 936ede8ef614..9361d076da76 100644 --- a/pkg/sql/schema_changer_test.go +++ b/pkg/sql/schema_changer_test.go @@ -8229,3 +8229,34 @@ DROP VIEW IF EXISTS v } wg.Wait() } + +func TestVirtualColumnNotAllowedInPkeyBefore22_1(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + + ctx := context.Background() + + params, _ := tests.CreateTestServerParams() + params.Knobs.Server = &server.TestingKnobs{ + DisableAutomaticVersionUpgrade: make(chan struct{}), + BinaryVersionOverride: clusterversion.ByKey(clusterversion.V21_2), + } + + s, sqlDB, _ := serverutils.StartServer(t, params) + defer s.Stopper().Stop(ctx) + + _, err := sqlDB.Exec(`CREATE TABLE t (a INT NOT NULL AS (1+1) VIRTUAL, PRIMARY KEY (a))`) + require.Error(t, err) + require.Equal(t, "pq: cannot use virtual column \"a\" in primary key", err.Error()) + + _, err = sqlDB.Exec(`CREATE TABLE t (a INT NOT NULL AS (1+1) VIRTUAL PRIMARY KEY)`) + require.Error(t, err) + require.Equal(t, "pq: cannot use virtual column \"a\" in primary key", err.Error()) + + _, err = sqlDB.Exec(`CREATE TABLE t (a INT PRIMARY KEY, b INT NOT NULL AS (1+1) VIRTUAL)`) + require.NoError(t, err) + + _, err = sqlDB.Exec(`ALTER TABLE t ALTER PRIMARY KEY USING COLUMNS (b)`) + require.Error(t, err) + require.Equal(t, "pq: cannot use virtual column \"b\" in primary key", err.Error()) +} diff --git a/pkg/workload/schemachange/BUILD.bazel b/pkg/workload/schemachange/BUILD.bazel index c3aa6eeaea10..e6e6b5843eb5 100644 --- a/pkg/workload/schemachange/BUILD.bazel +++ b/pkg/workload/schemachange/BUILD.bazel @@ -15,6 +15,8 @@ go_library( importpath = "github.com/cockroachdb/cockroach/pkg/workload/schemachange", visibility = ["//visibility:public"], deps = [ + "//pkg/clusterversion", + "//pkg/roachpb", "//pkg/security", "//pkg/sql/catalog/catpb", "//pkg/sql/catalog/colinfo", diff --git a/pkg/workload/schemachange/operation_generator.go b/pkg/workload/schemachange/operation_generator.go index 92c02699ff6f..e1364ee5fb7e 100644 --- a/pkg/workload/schemachange/operation_generator.go +++ b/pkg/workload/schemachange/operation_generator.go @@ -18,6 +18,8 @@ import ( "strings" "sync/atomic" + "github.com/cockroachdb/cockroach/pkg/clusterversion" + "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/security" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo" @@ -1116,9 +1118,36 @@ func (og *operationGenerator) createTable(ctx context.Context, tx pgx.Tx) (strin if err != nil { return "", err } + + // Detect if primary indexes contain computed columns, which are disallowed + // on older versions of Cockroach. + computedColInIndex := false + primaryKeyDisallowsComputedCols, err := isClusterVersionLessThan(ctx, tx, + clusterversion.TestingBinaryMinSupportedVersion) + if err != nil { + return "", err + } + if primaryKeyDisallowsComputedCols { + computedCols := make(map[string]struct{}) + for _, def := range stmt.Defs { + if colDef, ok := def.(*tree.ColumnTableDef); ok { + if colDef.IsVirtual() { + computedCols[colDef.Name.String()] = struct{}{} + } + } else if indexDef, ok := def.(*tree.IndexTableDef); ok { + for _, indexCol := range indexDef.Columns { + if _, ok := computedCols[indexCol.Column.String()]; ok { + computedColInIndex = true + } + } + } + } + } + codesWithConditions{ {code: pgcode.DuplicateRelation, condition: tableExists && !stmt.IfNotExists}, {code: pgcode.UndefinedSchema, condition: !schemaExists}, + {code: pgcode.FeatureNotSupported, condition: computedColInIndex}, }.add(og.expectedExecErrors) return tree.Serialize(stmt), nil @@ -3030,3 +3059,21 @@ func (og *operationGenerator) typeFromTypeName( } return typ, nil } + +// Check if the test is running with a mixed version cluster, with a version +// less than or equal to the target version number. This can be used to detect +// in mixed version environments if certain errors should be encountered. +func isClusterVersionLessThan( + ctx context.Context, tx pgx.Tx, targetVersion roachpb.Version, +) (bool, error) { + var clusterVersionStr string + row := tx.QueryRow(ctx, `SHOW CLUSTER SETTING version`) + if err := row.Scan(&clusterVersionStr); err != nil { + return false, err + } + clusterVersion, err := roachpb.ParseVersion(clusterVersionStr) + if err != nil { + return false, err + } + return clusterVersion.LessEq(targetVersion), nil +}