Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

opt: run sqlsmith roachtest queries with EXPLAIN #88333

Merged
merged 1 commit into from
Sep 21, 2022

Conversation

DrewKimball
Copy link
Collaborator

This commit modifies the SqlSmith roachtest to run each generated query with explain. This should allow the test to catch planning errors that do not happen during normal execution.

Informs #88037

Release note: None

@DrewKimball DrewKimball requested review from mgartner and a team September 21, 2022 10:10
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@michae2 michae2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @mgartner)

Copy link
Member

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained (waiting on @DrewKimball and @mgartner)


pkg/cmd/roachtest/tests/sqlsmith.go line 231 at r1 (raw file):

					if err == nil {
						logStmt(stmt)
						explainStmt := "EXPLAIN " + stmt

nit: I think we should do stmt = "EXPLAIN " + stmt here so that in case running the stmt is ok but explaining it is not we do see that it was the EXPLAIN that failed.

This commit modifies the SqlSmith roachtest to run each generated query
with explain. This should allow the test to catch planning errors that
do not happen during normal execution.

Informs cockroachdb#88037

Release note: None
Copy link
Collaborator Author

@DrewKimball DrewKimball left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TFTRs!

Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained (waiting on @mgartner)


pkg/cmd/roachtest/tests/sqlsmith.go line 231 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: I think we should do stmt = "EXPLAIN " + stmt here so that in case running the stmt is ok but explaining it is not we do see that it was the EXPLAIN that failed.

Done.

@DrewKimball
Copy link
Collaborator Author

DrewKimball commented Sep 21, 2022

Test failure seems unrelated, but maybe serious looks like that code is only hit during tests:

=== RUN   TestLogic_experimental_distsql_planning
    test_log_scope.go:161: test logs captured to: /go/src/github.com/cockroachdb/cockroach/artifacts/logTestLogic_experimental_distsql_planning3979966309
    test_log_scope.go:79: use -show-logs to present logs inline
*
* INFO: Running test with the default test tenant. If you are only seeing a test case failure when this message appears, there may be a problem with your test case running within tenants.
*
    panic.go:260: -- test log scope end --

ERROR: a panic has occurred!
Details cannot be printed yet because we are still unwinding.
Hopefully the test harness prints the panic below, otherwise check the test logs.

test logs left over in: /go/src/github.com/cockroachdb/cockroach/artifacts/logTestLogic_experimental_distsql_planning3979966309
    panic.go:260: runtime error: invalid memory address or nil pointer dereference
        goroutine 820655 [running]:
        runtime/debug.Stack()
        	/usr/local/go/src/runtime/debug/stack.go:24 +0x65
        github.com/cockroachdb/cockroach/pkg/util/leaktest.AfterTest.func1()
        	/go/src/github.com/cockroachdb/cockroach/pkg/util/leaktest/leaktest.go:110 +0x166
        panic({0x43202c0, 0x8365120})
        	/usr/local/go/src/runtime/panic.go:884 +0x212
        github.com/cockroachdb/cockroach/pkg/config.(*SystemConfig).PurgeZoneConfigCache(0x0)
        	/go/src/github.com/cockroachdb/cockroach/pkg/config/system.go:411 +0x33
        github.com/cockroachdb/cockroach/pkg/testutils/serverutils.StartNewTestCluster({_, _}, _, {{{{0x5cd69a0, 0xc004806000}, {0x0, 0x0}, {0x5cd6880, 0xc0063534a0}, {0x5cd7a40, ...}, ...}, ...}, ...})
        	/go/src/github.com/cockroachdb/cockroach/pkg/testutils/serverutils/test_cluster_shim.go:267 +0xeb
        github.com/cockroachdb/cockroach/pkg/sql/logictest.(*logicTest).newCluster(0xc000418f00, {0xc00005d958?, 0x0?, 0x0, 0x0?}, {0x0, 0x0, 0x3d28cc0?}, {0x0, 0x0, ...}, ...)
        	/go/src/github.com/cockroachdb/cockroach/pkg/sql/logictest/logic.go:1352 +0xde5
        github.com/cockroachdb/cockroach/pkg/sql/logictest.(*logicTest).setup(0xc000418f00, {{0x49dd412, 0xd}, 0x3, 0x1, {0x49c1310, 0x2}, 0x0, {0x0, 0x0}, ...}, ...)
        	/go/src/github.com/cockroachdb/cockroach/pkg/sql/logictest/logic.go:1702 +0x38d
        github.com/cockroachdb/cockroach/pkg/sql/logictest.RunLogicTest(0xc006286340, {0x20?, 0xc0?, 0xec?, 0x54?}, 0x6, {0xc0055be0a0, 0x4b})
        	/go/src/github.com/cockroachdb/cockroach/pkg/sql/logictest/logic.go:3941 +0x8ac
        github.com/cockroachdb/cockroach/pkg/sql/logictest/tests/fakedist-disk.runLogicTest(0xc006286340, {0x4a28df1, 0x1d})
        	/go/src/github.com/cockroachdb/cockroach/pkg/sql/logictest/tests/fakedist-disk/generated_test.go:58 +0x97
        github.com/cockroachdb/cockroach/pkg/sql/logictest/tests/fakedist-disk.TestLogic_experimental_distsql_planning(0xffb800?)
        	/go/src/github.com/cockroachdb/cockroach/pkg/sql/logictest/tests/fakedist-disk/generated_test.go:730 +0x59
        testing.tRunner(0xc006286340, 0x4c8e378)
        	/usr/local/go/src/testing/testing.go:1446 +0x10b
        created by testing.(*T).Run
        	/usr/local/go/src/testing/testing.go:1493 +0x35f
--- FAIL: TestLogic_experimental_distsql_planning (1.74s)

@DrewKimball
Copy link
Collaborator Author

bors r+

@craig
Copy link
Contributor

craig bot commented Sep 21, 2022

Build succeeded:

@craig craig bot merged commit c04fae2 into cockroachdb:master Sep 21, 2022
@DrewKimball DrewKimball deleted the smith-explain branch September 21, 2022 23:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants