Skip to content

Commit

Permalink
sql/schemachanger/scrun: add validation in makeState
Browse files Browse the repository at this point in the history
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
  • Loading branch information
ajwerner committed Apr 8, 2022
1 parent 958df7c commit d7fb91f
Show file tree
Hide file tree
Showing 9 changed files with 360 additions and 75 deletions.
1 change: 1 addition & 0 deletions pkg/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
42 changes: 28 additions & 14 deletions pkg/ccl/schemachangerccl/testdata/end_to_end/drop_multiregion
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ upsert descriptor #105
type:
arrayTypeId: 107
+ declarativeSchemaChangerState:
+ authorization: {}
+ authorization:
+ userName: root
+ jobId: "1"
enumMembers:
- logicalRepresentation: us-east1
Expand All @@ -46,7 +47,8 @@ upsert descriptor #107
family: ArrayFamily
oid: 100107
+ declarativeSchemaChangerState:
+ authorization: {}
+ authorization:
+ userName: root
+ jobId: "1"
id: 107
kind: ALIAS
Expand All @@ -62,7 +64,8 @@ upsert descriptor #108
createAsOfTime:
wallTime: "1"
+ declarativeSchemaChangerState:
+ authorization: {}
+ authorization:
+ userName: root
+ currentStatuses:
+ - ABSENT
+ - ABSENT
Expand Down Expand Up @@ -370,7 +373,8 @@ upsert descriptor #105
type:
arrayTypeId: 107
- declarativeSchemaChangerState:
- authorization: {}
- authorization:
- userName: root
- jobId: "1"
enumMembers:
- logicalRepresentation: us-east1
Expand All @@ -384,7 +388,8 @@ upsert descriptor #107
family: ArrayFamily
oid: 100107
- declarativeSchemaChangerState:
- authorization: {}
- authorization:
- userName: root
- jobId: "1"
id: 107
kind: ALIAS
Expand All @@ -398,7 +403,8 @@ upsert descriptor #108
createAsOfTime:
wallTime: "1"
- declarativeSchemaChangerState:
- authorization: {}
- authorization:
- userName: root
- currentStatuses:
- - ABSENT
- - ABSENT
Expand Down Expand Up @@ -720,7 +726,8 @@ upsert descriptor #105
type:
arrayTypeId: 107
+ declarativeSchemaChangerState:
+ authorization: {}
+ authorization:
+ userName: root
+ jobId: "1"
enumMembers:
- logicalRepresentation: us-east1
Expand All @@ -738,7 +745,8 @@ upsert descriptor #109
createAsOfTime:
wallTime: "1"
+ declarativeSchemaChangerState:
+ authorization: {}
+ authorization:
+ userName: root
+ currentStatuses:
+ - ABSENT
+ - ABSENT
Expand Down Expand Up @@ -954,7 +962,8 @@ upsert descriptor #105
type:
arrayTypeId: 107
- declarativeSchemaChangerState:
- authorization: {}
- authorization:
- userName: root
- jobId: "1"
enumMembers:
- logicalRepresentation: us-east1
Expand All @@ -968,7 +977,8 @@ upsert descriptor #109
createAsOfTime:
wallTime: "1"
- declarativeSchemaChangerState:
- authorization: {}
- authorization:
- userName: root
- currentStatuses:
- - ABSENT
- - ABSENT
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -1306,7 +1317,8 @@ upsert descriptor #105
type:
arrayTypeId: 107
+ declarativeSchemaChangerState:
+ authorization: {}
+ authorization:
+ userName: root
+ currentStatuses:
+ - ABSENT
+ - ABSENT
Expand Down Expand Up @@ -1403,7 +1415,8 @@ upsert descriptor #105
upsert descriptor #106
schema:
+ declarativeSchemaChangerState:
+ authorization: {}
+ authorization:
+ userName: root
+ currentStatuses:
+ - ABSENT
+ - ABSENT
Expand Down Expand Up @@ -1510,7 +1523,8 @@ upsert descriptor #107
family: ArrayFamily
oid: 100107
+ declarativeSchemaChangerState:
+ authorization: {}
+ authorization:
+ userName: root
+ currentStatuses:
+ - ABSENT
+ - ABSENT
Expand Down
14 changes: 7 additions & 7 deletions pkg/sql/schemachanger/scdeps/sctestdeps/database_state.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.")
Expand All @@ -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
}
20 changes: 19 additions & 1 deletion pkg/sql/schemachanger/scrun/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -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",
Expand All @@ -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",
Expand All @@ -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",
],
)
162 changes: 162 additions & 0 deletions pkg/sql/schemachanger/scrun/make_state_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
})
}
}
Loading

0 comments on commit d7fb91f

Please sign in to comment.