diff --git a/pkg/cmd/smithcmp/cmpconn/conn.go b/pkg/cmd/smithcmp/cmpconn/conn.go index af56b6012339..86acf54c8af0 100644 --- a/pkg/cmd/smithcmp/cmpconn/conn.go +++ b/pkg/cmd/smithcmp/cmpconn/conn.go @@ -16,6 +16,7 @@ import ( gosql "database/sql" "fmt" "math/rand" + "strings" "time" "github.com/cockroachdb/cockroach/pkg/sql/mutations" @@ -98,17 +99,21 @@ func (c *Conn) Exec(ctx context.Context, s string) error { } // Values executes prep and exec and returns the results of exec. Mutators -// passed in during NewConn are applied only to exec. -func (c *Conn) Values(ctx context.Context, prep, exec string) (*pgx.Rows, error) { +// passed in during NewConn are applied only to exec. The mutated exec string +// is returned. +func (c *Conn) Values( + ctx context.Context, prep, exec string, +) (rows *pgx.Rows, mutated string, err error) { if prep != "" { - rows, err := c.PGX.QueryEx(ctx, prep, simpleProtocol) + rows, err = c.PGX.QueryEx(ctx, prep, simpleProtocol) if err != nil { - return nil, err + return nil, "", err } rows.Close() } - exec, _ = mutations.ApplyString(c.rng, exec, c.sqlMutators...) - return c.PGX.QueryEx(ctx, exec, simpleProtocol) + mutated, _ = mutations.ApplyString(c.rng, exec, c.sqlMutators...) + rows, err = c.PGX.QueryEx(ctx, mutated, simpleProtocol) + return rows, mutated, err } var simpleProtocol = &pgx.QueryExOptions{SimpleProtocol: true} @@ -117,18 +122,41 @@ var simpleProtocol = &pgx.QueryExOptions{SimpleProtocol: true} // differ, an error is returned. SQL errors are ignored. func CompareConns( ctx context.Context, timeout time.Duration, conns map[string]*Conn, prep, exec string, -) error { +) (err error) { ctx, cancel := context.WithTimeout(ctx, timeout) defer cancel() connRows := make(map[string]*pgx.Rows) + connExecs := make(map[string]string) for name, conn := range conns { - rows, err := conn.Values(ctx, prep, exec) + rows, mutated, err := conn.Values(ctx, prep, exec) if err != nil { return nil //nolint:returnerrcheck } defer rows.Close() connRows[name] = rows + connExecs[name] = mutated } + + // Annotate our error message with the exec queries since they can be + // mutated and differ per connection. + defer func() { + if err == nil { + return + } + var sb strings.Builder + prev := "" + for name, mutated := range connExecs { + fmt.Fprintf(&sb, "\n%s:", name) + if prev == mutated { + sb.WriteString(" [same as previous]\n") + } else { + fmt.Fprintf(&sb, "\n%s;\n", mutated) + } + prev = mutated + } + err = fmt.Errorf("%w%s", err, sb.String()) + }() + var first []interface{} var firstName string var minCount int diff --git a/pkg/compose/compare/compare/compare_test.go b/pkg/compose/compare/compare/compare_test.go index 46e96fab8693..805974a51c68 100644 --- a/pkg/compose/compare/compare/compare_test.go +++ b/pkg/compose/compare/compare/compare_test.go @@ -19,6 +19,7 @@ import ( "context" "flag" "io/ioutil" + "path/filepath" "strings" "testing" "time" @@ -30,21 +31,47 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/randutil" ) -var flagEach = flag.Duration("each", 10*time.Minute, "individual test timeout") +var ( + flagEach = flag.Duration("each", 10*time.Minute, "individual test timeout") + flagArtifacts = flag.String("artifacts", "", "artifact directory") +) func TestCompare(t *testing.T) { - uris := map[string]string{ - "postgres": "postgresql://postgres@postgres:5432/", - "cockroach1": "postgresql://root@cockroach1:26257/?sslmode=disable", - "cockroach2": "postgresql://root@cockroach2:26257/?sslmode=disable", + uris := map[string]struct { + addr string + init []string + }{ + "postgres": { + addr: "postgresql://postgres@postgres:5432/", + init: []string{ + "drop schema if exists public cascade", + "create schema public", + }, + }, + "cockroach1": { + addr: "postgresql://root@cockroach1:26257/?sslmode=disable", + init: []string{ + "drop database if exists defaultdb", + "create database defaultdb", + }, + }, + "cockroach2": { + addr: "postgresql://root@cockroach2:26257/?sslmode=disable", + init: []string{ + "drop database if exists defaultdb", + "create database defaultdb", + }, + }, } configs := map[string]testConfig{ - "default": { - opts: []sqlsmith.SmitherOption{sqlsmith.PostgresMode()}, + "postgres": { + setup: sqlsmith.Setups["rand-tables"], + setupMutators: []sqlbase.Mutator{mutations.PostgresCreateTableMutator}, + opts: []sqlsmith.SmitherOption{sqlsmith.PostgresMode()}, conns: []testConn{ { name: "cockroach1", - mutators: []sqlbase.Mutator{mutations.PostgresMutator}, + mutators: []sqlbase.Mutator{}, }, { name: "postgres", @@ -52,29 +79,63 @@ func TestCompare(t *testing.T) { }, }, }, + "mutators": { + setup: sqlsmith.Setups["rand-tables"], + opts: []sqlsmith.SmitherOption{sqlsmith.CompareMode()}, + conns: []testConn{ + { + name: "cockroach1", + mutators: []sqlbase.Mutator{}, + }, + { + name: "cockroach2", + mutators: []sqlbase.Mutator{ + mutations.StatisticsMutator, + mutations.ForeignKeyMutator, + mutations.ColumnFamilyMutator, + mutations.StatisticsMutator, + mutations.IndexStoringMutator, + }, + }, + }, + }, } ctx := context.Background() for confName, config := range configs { t.Run(confName, func(t *testing.T) { rng, _ := randutil.NewPseudoRand() + setup := config.setup(rng) + setup, _ = mutations.ApplyString(rng, setup, config.setupMutators...) + conns := map[string]*cmpconn.Conn{} for _, testCn := range config.conns { uri, ok := uris[testCn.name] if !ok { t.Fatalf("bad connection name: %s", testCn.name) } - conn, err := cmpconn.NewConn(uri, rng, testCn.mutators) + conn, err := cmpconn.NewConn(uri.addr, rng, testCn.mutators) if err != nil { t.Fatal(err) } defer conn.Close() + for _, init := range uri.init { + if err := conn.Exec(ctx, init); err != nil { + t.Fatalf("%s: %v", testCn.name, err) + } + } + connSetup, _ := mutations.ApplyString(rng, setup, testCn.mutators...) + if err := conn.Exec(ctx, connSetup); err != nil { + t.Log(connSetup) + t.Fatalf("%s: %v", testCn.name, err) + } conns[testCn.name] = conn } smither, err := sqlsmith.NewSmither(conns[config.conns[0].name].DB, rng, config.opts...) if err != nil { t.Fatal(err) } + until := time.After(*flagEach) for { select { @@ -93,8 +154,10 @@ func TestCompare(t *testing.T) { } query, _ = mutations.ApplyString(rng, query, mutations.PostgresMutator) if err := cmpconn.CompareConns(ctx, time.Second*30, conns, "" /* prep */, query); err != nil { - _ = ioutil.WriteFile("/test/query.sql", []byte(query+";"), 0666) - t.Log(query) + path := filepath.Join(*flagArtifacts, confName+".log") + if err := ioutil.WriteFile(path, []byte(err.Error()), 0666); err != nil { + t.Log(err) + } t.Fatal(err) } // Make sure we can still ping on a connection. If we can't we may have @@ -111,8 +174,10 @@ func TestCompare(t *testing.T) { } type testConfig struct { - opts []sqlsmith.SmitherOption - conns []testConn + opts []sqlsmith.SmitherOption + conns []testConn + setup sqlsmith.Setup + setupMutators []sqlbase.Mutator } type testConn struct { diff --git a/pkg/compose/compare/docker-compose.yml b/pkg/compose/compare/docker-compose.yml index 75fead9a52fe..9232bc29d97f 100644 --- a/pkg/compose/compare/docker-compose.yml +++ b/pkg/compose/compare/docker-compose.yml @@ -17,7 +17,7 @@ services: test: image: ubuntu:xenial-20170214 # compare.test is a binary built by the pkg/compose/prepare.sh - command: /compare/compare.test -each ${EACH} + command: /compare/compare.test -each ${EACH} -test.run ${TESTS} -artifacts /compare depends_on: - postgres - cockroach1 diff --git a/pkg/compose/compose_test.go b/pkg/compose/compose_test.go index 43a500e8db6a..27157151e2e1 100644 --- a/pkg/compose/compose_test.go +++ b/pkg/compose/compose_test.go @@ -20,7 +20,10 @@ import ( "time" ) -var flagEach = flag.Duration("each", 10*time.Minute, "individual test timeout") +var ( + flagEach = flag.Duration("each", 10*time.Minute, "individual test timeout") + flagTests = flag.String("tests", ".", "tests within docker compose to run") +) func TestComposeCompare(t *testing.T) { const file = "docker-compose" @@ -34,7 +37,10 @@ func TestComposeCompare(t *testing.T) { "--force-recreate", "--exit-code-from", "test", ) - cmd.Env = []string{fmt.Sprintf("EACH=%s", *flagEach)} + cmd.Env = []string{ + fmt.Sprintf("EACH=%s", *flagEach), + fmt.Sprintf("TESTS=%s", *flagTests), + } out, err := cmd.CombinedOutput() if err != nil { t.Log(string(out)) diff --git a/pkg/internal/sqlsmith/sqlsmith.go b/pkg/internal/sqlsmith/sqlsmith.go index 2a4716302652..f74d5704c063 100644 --- a/pkg/internal/sqlsmith/sqlsmith.go +++ b/pkg/internal/sqlsmith/sqlsmith.go @@ -330,6 +330,8 @@ var CompareMode = multiOption( "compare mode", DisableMutations(), DisableImpureFns(), + DisableCRDBFns(), + IgnoreFNs("^version"), DisableLimits(), OutputSort(), ) @@ -340,10 +342,8 @@ var PostgresMode = multiOption( "postgres mode", CompareMode(), DisableWith(), - DisableCRDBFns(), SimpleDatums(), IgnoreFNs("^current_"), - IgnoreFNs("^version"), simpleOption("postgres", func(s *Smither) { s.postgres = true })(), diff --git a/pkg/sql/mutations/mutations.go b/pkg/sql/mutations/mutations.go index 42f5b055ed1b..6c47db9daeea 100644 --- a/pkg/sql/mutations/mutations.go +++ b/pkg/sql/mutations/mutations.go @@ -35,9 +35,28 @@ var ( // definitions to have random FAMILY definitions. ColumnFamilyMutator StatementMutator = sqlbase.ColumnFamilyMutator + // IndexStoringMutator modifies the STORING clause of CREATE INDEX and + // indexes in CREATE TABLE. + IndexStoringMutator MultiStatementMutation = sqlbase.IndexStoringMutator + // PostgresMutator modifies strings such that they execute identically - // in both Postgres and Cockroach. + // in both Postgres and Cockroach (however this mutator does not remove + // features not supported by Postgres; use PostgresCreateTableMutator + // for those). PostgresMutator StatementStringMutator = postgresMutator + + // PostgresCreateTableMutator modifies CREATE TABLE statements to + // remove any features not supported by Postgres that would change + // results (like descending primary keys). This should be used on the + // output of sqlbase.RandCreateTable. + PostgresCreateTableMutator MultiStatementMutation = postgresCreateTableMutator +) + +var ( + // These are used in pkg/compose/compare/compare/compare_test.go, but + // it has a build tag so it's not detected by the linter. + _ = IndexStoringMutator + _ = PostgresCreateTableMutator ) // StatementMutator defines a func that can change a statement. @@ -312,6 +331,9 @@ func foreignKeyMutator( } } } + if len(tables) == 0 { + return stmts, false + } toNames := func(cols []*tree.ColumnTableDef) tree.NameList { names := make(tree.NameList, len(cols)) @@ -485,7 +507,9 @@ Loop: } } -func postgresMutator(_ *rand.Rand, q string) string { +func postgresMutator(rng *rand.Rand, q string) string { + q, _ = ApplyString(rng, q, postgresStatementMutator) + for from, to := range map[string]string{ ":::": "::", "STRING": "TEXT", @@ -498,3 +522,125 @@ func postgresMutator(_ *rand.Rand, q string) string { } return q } + +// postgresStatementMutator removes cockroach-only things from CREATE TABLE and +// ALTER TABLE. +var postgresStatementMutator MultiStatementMutation = func(rng *rand.Rand, stmts []tree.Statement) (mutated []tree.Statement, changed bool) { + for _, stmt := range stmts { + switch stmt := stmt.(type) { + case *tree.SetClusterSetting: + continue + case *tree.CreateTable: + if stmt.Interleave != nil { + stmt.Interleave = nil + changed = true + } + if stmt.PartitionBy != nil { + stmt.PartitionBy = nil + changed = true + } + for i := 0; i < len(stmt.Defs); i++ { + switch def := stmt.Defs[i].(type) { + case *tree.FamilyTableDef: + // Remove. + stmt.Defs = append(stmt.Defs[:i], stmt.Defs[i+1:]...) + i-- + changed = true + case *tree.ColumnTableDef: + if def.HasColumnFamily() { + def.Family.Name = "" + def.Family.Create = false + changed = true + } + case *tree.UniqueConstraintTableDef: + if def.Interleave != nil { + def.Interleave = nil + changed = true + } + if def.PartitionBy != nil { + def.PartitionBy = nil + changed = true + } + } + } + case *tree.AlterTable: + for i := 0; i < len(stmt.Cmds); i++ { + // Postgres doesn't have alter stats. + if _, ok := stmt.Cmds[i].(*tree.AlterTableInjectStats); ok { + stmt.Cmds = append(stmt.Cmds[:i], stmt.Cmds[i+1:]...) + i-- + changed = true + } + } + // If there are no commands, don't add this statement. + if len(stmt.Cmds) == 0 { + continue + } + } + mutated = append(mutated, stmt) + } + return mutated, changed +} + +func postgresCreateTableMutator( + rng *rand.Rand, stmts []tree.Statement, +) (mutated []tree.Statement, changed bool) { + for _, stmt := range stmts { + mutated = append(mutated, stmt) + switch stmt := stmt.(type) { + case *tree.CreateTable: + var newdefs tree.TableDefs + for _, def := range stmt.Defs { + switch def := def.(type) { + case *tree.IndexTableDef: + // Postgres doesn't support + // indexes in CREATE TABLE, + // so split them out to their + // own statement. + mutated = append(mutated, &tree.CreateIndex{ + Name: def.Name, + Table: stmt.Table, + Inverted: def.Inverted, + Columns: def.Columns, + Storing: def.Storing, + }) + changed = true + case *tree.UniqueConstraintTableDef: + if def.PrimaryKey { + // Postgres doesn't support descending PKs. + for i, col := range def.Columns { + if col.Direction != tree.DefaultDirection { + def.Columns[i].Direction = tree.DefaultDirection + changed = true + } + } + if def.Name != "" { + // Unset Name here because + // constaint names cannot + // be shared among tables, + // so multiple PK constraints + // named "primary" is an error. + def.Name = "" + changed = true + } + newdefs = append(newdefs, def) + break + } + mutated = append(mutated, &tree.CreateIndex{ + Name: def.Name, + Table: stmt.Table, + Unique: true, + Inverted: def.Inverted, + Columns: def.Columns, + Storing: def.Storing, + }) + changed = true + default: + newdefs = append(newdefs, def) + } + } + stmt.Defs = newdefs + } + } + return mutated, changed +} diff --git a/pkg/sql/mutations/mutations_test.go b/pkg/sql/mutations/mutations_test.go new file mode 100644 index 000000000000..3101efe4fba3 --- /dev/null +++ b/pkg/sql/mutations/mutations_test.go @@ -0,0 +1,54 @@ +// Copyright 2020 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 mutations + +import ( + "strings" + "testing" + + "github.com/cockroachdb/cockroach/pkg/util/randutil" +) + +func TestPostgresMutator(t *testing.T) { + q := ` + CREATE TABLE t (s STRING FAMILY fam1, b BYTES, FAMILY fam2 (b), PRIMARY KEY (s ASC, b DESC)) + PARTITION BY LIST (s) + ( + PARTITION europe_west VALUES IN ('a', 'b') + ); + ALTER TABLE table1 INJECT STATISTICS 'blah'; + SET CLUSTER SETTING "sql.stats.automatic_collection.enabled" = false; + ` + + rng, _ := randutil.NewPseudoRand() + { + mutated, changed := ApplyString(rng, q, PostgresMutator) + if !changed { + t.Fatal("expected changed") + } + mutated = strings.TrimSpace(mutated) + expect := `CREATE TABLE t (s TEXT, b BYTEA, PRIMARY KEY (s ASC, b DESC));` + if mutated != expect { + t.Fatalf("unexpected: %s", mutated) + } + } + { + mutated, changed := ApplyString(rng, q, PostgresCreateTableMutator, PostgresMutator) + if !changed { + t.Fatal("expected changed") + } + mutated = strings.TrimSpace(mutated) + expect := `CREATE TABLE t (s TEXT, b BYTEA, PRIMARY KEY (s, b));` + if mutated != expect { + t.Fatalf("unexpected: %s", mutated) + } + } +}