From b9b70c1a50d00ea8a1e14ae23b64d6412bc656b8 Mon Sep 17 00:00:00 2001 From: Neha George Date: Wed, 29 Sep 2021 11:29:34 -0400 Subject: [PATCH 1/2] sqlsmith, tests: compare result sets in TLP testing Previously, only the result counts of the unpartitioned and partitioned TLP queries were compared. This was inadequate because it allowed for different rows values in the two result sets to go undetected, as long as the row counts were the same. To address this, the rows in the result sets are compared directly to ensure they are the same, as expected. Release note: None --- pkg/cmd/roachtest/tests/BUILD.bazel | 1 + pkg/cmd/roachtest/tests/tlp.go | 49 +++++++++++++---- pkg/internal/sqlsmith/tlp.go | 82 ++++++++++++----------------- 3 files changed, 74 insertions(+), 58 deletions(-) diff --git a/pkg/cmd/roachtest/tests/BUILD.bazel b/pkg/cmd/roachtest/tests/BUILD.bazel index 864439acc653..c550f41cca3f 100644 --- a/pkg/cmd/roachtest/tests/BUILD.bazel +++ b/pkg/cmd/roachtest/tests/BUILD.bazel @@ -184,6 +184,7 @@ go_library( "@com_github_codahale_hdrhistogram//:hdrhistogram", "@com_github_dustin_go_humanize//:go-humanize", "@com_github_golang_mock//gomock", + "@com_github_google_go_cmp//cmp", "@com_github_jackc_pgtype//:pgtype", "@com_github_kr_pretty//:pretty", "@com_github_lib_pq//:pq", diff --git a/pkg/cmd/roachtest/tests/tlp.go b/pkg/cmd/roachtest/tests/tlp.go index e0464ead749e..180b8f5a6074 100644 --- a/pkg/cmd/roachtest/tests/tlp.go +++ b/pkg/cmd/roachtest/tests/tlp.go @@ -16,6 +16,7 @@ import ( "fmt" "os" "path/filepath" + "sort" "strings" "time" @@ -23,8 +24,10 @@ import ( "github.com/cockroachdb/cockroach/pkg/cmd/roachtest/registry" "github.com/cockroachdb/cockroach/pkg/cmd/roachtest/test" "github.com/cockroachdb/cockroach/pkg/internal/sqlsmith" + "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" "github.com/cockroachdb/cockroach/pkg/util/randutil" "github.com/cockroachdb/errors" + "github.com/google/go-cmp/cmp" ) const statementTimeout = time.Minute @@ -169,34 +172,58 @@ func runTLPQuery(conn *gosql.DB, smither *sqlsmith.Smither, logStmt func(string) unpartitioned, partitioned := smither.GenerateTLP() return runWithTimeout(func() error { - var unpartitionedCount int - row := conn.QueryRow(unpartitioned) - if err := row.Scan(&unpartitionedCount); err != nil { + rows1, err := conn.Query(unpartitioned) + if err != nil { // Ignore errors. //nolint:returnerrcheck return nil } - - var partitionedCount int - row = conn.QueryRow(partitioned) - if err := row.Scan(&partitionedCount); err != nil { + defer rows1.Close() + unpartitionedRows, err := sqlutils.RowsToStrMatrix(rows1) + if err != nil { + // Ignore errors. + //nolint:returnerrcheck + return nil + } + rows2, err := conn.Query(partitioned) + if err != nil { + // Ignore errors. + //nolint:returnerrcheck + return nil + } + defer rows2.Close() + partitionedRows, err := sqlutils.RowsToStrMatrix(rows2) + if err != nil { // Ignore errors. //nolint:returnerrcheck return nil } - if unpartitionedCount != partitionedCount { + if diff := unsortedMatricesDiff(unpartitionedRows, partitionedRows); diff != "" { logStmt(unpartitioned) logStmt(partitioned) return errors.Newf( - "expected unpartitioned count %d to equal partitioned count %d\nsql: %s\n%s", - unpartitionedCount, partitionedCount, unpartitioned, partitioned) + "expected unpartitioned results to equal partitioned results\n%s\nsql: %s\n%s", + diff, unpartitioned, partitioned) } - return nil }) } +func unsortedMatricesDiff(rowMatrix1, rowMatrix2 [][]string) string { + var rows1 []string + for _, row := range rowMatrix1 { + rows1 = append(rows1, strings.Join(row[:], ",")) + } + var rows2 []string + for _, row := range rowMatrix2 { + rows2 = append(rows2, strings.Join(row[:], ",")) + } + sort.Strings(rows1) + sort.Strings(rows2) + return cmp.Diff(rows1, rows2) +} + func runWithTimeout(f func() error) error { done := make(chan error, 1) go func() { diff --git a/pkg/internal/sqlsmith/tlp.go b/pkg/internal/sqlsmith/tlp.go index db627d20e004..e0c23835d8cb 100644 --- a/pkg/internal/sqlsmith/tlp.go +++ b/pkg/internal/sqlsmith/tlp.go @@ -28,14 +28,10 @@ import ( // // More information on TLP: https://www.manuelrigger.at/preprints/TLP.pdf. // -// We currently implement a limited form of TLP that can only verify that the -// number of rows returned by the unpartitioned and the partitioned queries are -// equal. -// -// This TLP implementation is also limited in the types of queries that are -// tested. We currently only test basic WHERE and JOIN query filters. It is -// possible to use TLP to test aggregations, GROUP BY, and HAVING, which have -// all been implemented in SQLancer. See: +// This TLP implementation is limited in the types of queries that are tested. +// We currently only test basic WHERE, JOIN, and MAX/MIN query filters. It is +// possible to use TLP to test other aggregations, GROUP BY, and HAVING, which +// have all been implemented in SQLancer. See: // https://github.com/sqlancer/sqlancer/tree/1.1.0/src/sqlancer/cockroachdb/oracle/tlp. func (s *Smither) GenerateTLP() (unpartitioned, partitioned string) { // Set disableImpureFns to true so that generated predicates are immutable. @@ -63,19 +59,17 @@ func (s *Smither) GenerateTLP() (unpartitioned, partitioned string) { // // The first query returned is an unpartitioned query of the form: // -// SELECT count(*) FROM table +// SELECT * FROM table // // The second query returned is a partitioned query of the form: // -// SELECT count(*) FROM ( -// SELECT * FROM table WHERE (p) -// UNION ALL -// SELECT * FROM table WHERE NOT (p) -// UNION ALL -// SELECT * FROM table WHERE (p) IS NULL -// ) +// SELECT * FROM table WHERE (p) +// UNION ALL +// SELECT * FROM table WHERE NOT (p) +// UNION ALL +// SELECT * FROM table WHERE (p) IS NULL // -// If the resulting counts of the two queries are not equal, there is a logical +// If the resulting values of the two queries are not equal, there is a logical // bug. func (s *Smither) generateWhereTLP() (unpartitioned, partitioned string) { f := tree.NewFmtCtx(tree.FmtParsable) @@ -87,7 +81,7 @@ func (s *Smither) generateWhereTLP() (unpartitioned, partitioned string) { table.Format(f) tableName := f.CloseAndGetString() - unpartitioned = fmt.Sprintf("SELECT count(*) FROM %s", tableName) + unpartitioned = fmt.Sprintf("SELECT * FROM %s", tableName) pred := makeBoolExpr(s, cols) pred.Format(f) @@ -98,7 +92,7 @@ func (s *Smither) generateWhereTLP() (unpartitioned, partitioned string) { part3 := fmt.Sprintf("SELECT * FROM %s WHERE (%s) IS NULL", tableName, predicate) partitioned = fmt.Sprintf( - "SELECT count(*) FROM (%s UNION ALL %s UNION ALL %s)", + "(%s) UNION ALL (%s) UNION ALL (%s)", part1, part2, part3, ) @@ -112,23 +106,19 @@ func (s *Smither) generateWhereTLP() (unpartitioned, partitioned string) { // // The first query returned is an unpartitioned query of the form: // -// SELECT count(*) FROM ( -// SELECT * FROM table1 LEFT JOIN table2 ON TRUE -// UNION ALL -// SELECT * FROM table1 LEFT JOIN table2 ON FALSE -// UNION ALL -// SELECT * FROM table1 LEFT JOIN table2 ON FALSE -// ) +// SELECT * FROM table1 LEFT JOIN table2 ON TRUE +// UNION ALL +// SELECT * FROM table1 LEFT JOIN table2 ON FALSE +// UNION ALL +// SELECT * FROM table1 LEFT JOIN table2 ON FALSE // // The second query returned is a partitioned query of the form: // -// SELECT count(*) FROM ( -// SELECT * FROM table1 LEFT JOIN table2 ON (p) -// UNION ALL -// SELECT * FROM table1 LEFT JOIN table2 ON NOT (p) -// UNION ALL -// SELECT * FROM table1 LEFT JOIN table2 ON (p) IS NULL -// ) +// SELECT * FROM table1 LEFT JOIN table2 ON (p) +// UNION ALL +// SELECT * FROM table1 LEFT JOIN table2 ON NOT (p) +// UNION ALL +// SELECT * FROM table1 LEFT JOIN table2 ON (p) IS NULL // // From the first query, we have a CROSS JOIN of the two tables (JOIN ON TRUE) // and then all rows concatenated with NULL values for the second and third @@ -144,7 +134,7 @@ func (s *Smither) generateWhereTLP() (unpartitioned, partitioned string) { // Note that this implementation is restricted in that it only uses columns from // the left table in the predicate p. -// If the resulting counts of the two queries are not equal, there is a logical +// If the resulting values of the two queries are not equal, there is a logical // bug. func (s *Smither) generateOuterJoinTLP() (unpartitioned, partitioned string) { f := tree.NewFmtCtx(tree.FmtParsable) @@ -169,7 +159,7 @@ func (s *Smither) generateOuterJoinTLP() (unpartitioned, partitioned string) { ) unpartitioned = fmt.Sprintf( - "SELECT count(*) FROM (%s UNION ALL %s UNION ALL %s)", + "(%s) UNION ALL (%s) UNION ALL (%s)", leftJoinTrue, leftJoinFalse, leftJoinFalse, ) @@ -191,7 +181,7 @@ func (s *Smither) generateOuterJoinTLP() (unpartitioned, partitioned string) { ) partitioned = fmt.Sprintf( - "SELECT count(*) FROM (%s UNION ALL %s UNION ALL %s)", + "(%s) UNION ALL (%s) UNION ALL (%s)", part1, part2, part3, ) @@ -205,17 +195,15 @@ func (s *Smither) generateOuterJoinTLP() (unpartitioned, partitioned string) { // // The first query returned is an unpartitioned query of the form: // -// SELECT count(*) FROM table1 JOIN table2 ON TRUE +// SELECT * FROM table1 JOIN table2 ON TRUE // // The second query returned is a partitioned query of the form: // -// SELECT count(*) FROM ( -// SELECT * FROM table1 JOIN table2 ON (p) -// UNION ALL -// SELECT * FROM table1 JOIN table2 ON NOT (p) -// UNION ALL -// SELECT * FROM table1 JOIN table2 ON (p) IS NULL -// ) +// SELECT * FROM table1 JOIN table2 ON (p) +// UNION ALL +// SELECT * FROM table1 JOIN table2 ON NOT (p) +// UNION ALL +// SELECT * FROM table1 JOIN table2 ON (p) IS NULL // // From the first query, we have a CROSS JOIN of the two tables (JOIN ON TRUE). // Recall our TLP logical guarantee that a given predicate p always evaluates to @@ -224,7 +212,7 @@ func (s *Smither) generateOuterJoinTLP() (unpartitioned, partitioned string) { // resolve to TRUE. So the partitioned query accounts for each row in the // CROSS JOIN exactly once. // -// If the resulting counts of the two queries are not equal, there is a logical +// If the resulting values of the two queries are not equal, there is a logical // bug. func (s *Smither) generateInnerJoinTLP() (unpartitioned, partitioned string) { f := tree.NewFmtCtx(tree.FmtParsable) @@ -240,7 +228,7 @@ func (s *Smither) generateInnerJoinTLP() (unpartitioned, partitioned string) { tableName2 := f.CloseAndGetString() unpartitioned = fmt.Sprintf( - "SELECT count(*) FROM %s JOIN %s ON true", + "SELECT * FROM %s JOIN %s ON true", tableName1, tableName2, ) @@ -263,7 +251,7 @@ func (s *Smither) generateInnerJoinTLP() (unpartitioned, partitioned string) { ) partitioned = fmt.Sprintf( - "SELECT count(*) FROM (%s UNION ALL %s UNION ALL %s)", + "(%s) UNION ALL (%s) UNION ALL (%s)", part1, part2, part3, ) From f6de30e06336672509ce05558e9112b7bb4b96f9 Mon Sep 17 00:00:00 2001 From: Rafi Shamim Date: Thu, 30 Sep 2021 16:47:57 -0400 Subject: [PATCH 2/2] sql: deflake TestCancelQueryPermissions Release note: None --- pkg/sql/run_control_test.go | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/pkg/sql/run_control_test.go b/pkg/sql/run_control_test.go index f87c17bbeedd..a25328081e06 100644 --- a/pkg/sql/run_control_test.go +++ b/pkg/sql/run_control_test.go @@ -17,6 +17,7 @@ import ( "math/rand" "net/url" "strings" + "sync" "testing" "time" @@ -25,7 +26,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/sqltestutils" "github.com/cockroachdb/cockroach/pkg/testutils" "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" - "github.com/cockroachdb/cockroach/pkg/testutils/skip" "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/cockroachdb/cockroach/pkg/util/log" @@ -242,7 +242,6 @@ GRANT admin TO has_admin2; func TestCancelQueryPermissions(t *testing.T) { defer leaktest.AfterTest(t)() - skip.WithIssue(t, 63547, "https://cockroachlabs.slack.com/archives/C016CAD2HQ8/p1627290710022700") defer log.Scope(t).Close(t) // getQueryIDs retrieves the IDs of any currently running queries for the @@ -312,8 +311,13 @@ GRANT admin TO has_admin2; {"unpermissioned users cannot cancel other users", "no_perms", "has_cancelquery", false, "this operation requires CANCELQUERY privilege"}, } + // Avoid using subtests with t.Run since we may need to access `t` after the + // subtest is done. Use a WaitGroup to make sure the error from the pg_sleep + // goroutine is checked. + wg := sync.WaitGroup{} for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { + func() { + wg.Add(1) // Start a query with the target user. targetDB := getUserConn(t, tc.targetUser, testCluster.Server(0)) defer targetDB.Close() @@ -326,7 +330,11 @@ GRANT admin TO has_admin2; // end of the test. errRE = "sql: database is closed" } - sqlutils.MakeSQLRunner(targetDB).ExpectErr(t, errRE, "SELECT pg_sleep(1000000)") + _, err := targetDB.ExecContext(context.Background(), "SELECT pg_sleep(100)") + if !testutils.IsError(err, errRE) { + t.Errorf("expected error '%s', got: %v", errRE, err) + } + wg.Done() }() // Retrieve the query ID. @@ -353,8 +361,10 @@ GRANT admin TO has_admin2; } else { runner.ExpectErr(t, tc.expectedErrRE, `CANCEL QUERY $1`, queryID) } - }) + }() } + testCluster.Stopper().Stop(ctx) + wg.Wait() } func TestCancelIfExists(t *testing.T) {