Skip to content

Commit

Permalink
Merge #70868 #70951
Browse files Browse the repository at this point in the history 
70868: sqlsmith, tests: Compare result sets in TLP testing r=nehageorge a=nehageorge

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

70951: sql: deflake TestCancelQueryPermissions r=otan a=rafiss

fixes #63547

Release note: None

Co-authored-by: Neha George <[email protected]>
Co-authored-by: Rafi Shamim <[email protected]>
  • Loading branch information
@rafiss
3 people committed Sep 30, 2021
3 parents a7f7e22 + b9b70c1 + f6de30e commit aa7b70b
Show file tree
Hide file tree
Showing 4 changed files with 89 additions and 63 deletions.
1 change: 1 addition & 0 deletions pkg/cmd/roachtest/tests/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
49 changes: 38 additions & 11 deletions pkg/cmd/roachtest/tests/tlp.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,18 @@ import (
"fmt"
"os"
"path/filepath"
"sort"
"strings"
"time"

"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/cluster"
"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
Expand Down Expand Up @@ -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() {
Expand Down
82 changes: 35 additions & 47 deletions pkg/internal/sqlsmith/tlp.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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,
)

Expand All @@ -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
Expand All @@ -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)
Expand All @@ -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,
)

Expand All @@ -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,
)

Expand All @@ -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
Expand All @@ -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)
Expand All @@ -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,
)

Expand All @@ -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,
)

Expand Down
20 changes: 15 additions & 5 deletions pkg/sql/run_control_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"math/rand"
"net/url"
"strings"
"sync"
"testing"
"time"

Expand All @@ -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"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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()
Expand All @@ -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.
Expand All @@ -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) {
Expand Down

0 comments on commit aa7b70b

Please sign in to comment.