Merge cockroachdb#44364

44364: compose/compare: add tables to comparison tests r=mjibson a=mjibson This involves quite a lot of mutation code to coerce tables created with RandCreateTable into something that 1) postgres can executed and 2) will execute identically between cockroach and postgres while keeping the cockroach-specific features in the tables. Some of these features don't affect query output, but should still be tested in cockroach (i.e., column families, interleave). Co-authored-by: Matt Jibson <[email protected]>
rohany · Jan 25, 2020 · 72ce657 · 72ce657
2 parents 7d63e8e + f1df3b4
commit 72ce657
Show file tree

Hide file tree

Showing 7 changed files with 327 additions and 28 deletions.
diff --git 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
@@ -19,6 +19,7 @@ import (
 	"context"
 	"flag"
 	"io/ioutil"
+	"path/filepath"
 	"strings"
 	"testing"
 	"time"
@@ -30,51 +31,111 @@ 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",
 					mutators: []sqlbase.Mutator{mutations.PostgresMutator},
 				},
 			},
 		},
+		"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
@@ -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
@@ -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
@@ -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
 	})(),