Skip to content

opt: run sqlsmith roachtest queries with EXPLAIN#88333

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
DrewKimball:smith-explain
Sep 21, 2022
Merged

opt: run sqlsmith roachtest queries with EXPLAIN#88333
craig[bot] merged 1 commit intocockroachdb:masterfrom
DrewKimball:smith-explain

Conversation

@DrewKimball
Copy link
Copy Markdown
Collaborator

@DrewKimball DrewKimball commented Sep 21, 2022

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 a team and mgartner September 21, 2022 10:10
@cockroach-teamcity
Copy link
Copy Markdown
Member

cockroach-teamcity commented Sep 21, 2022

This change is Reviewable

michae2
michae2 approved these changes Sep 21, 2022
View reviewed changes
Copy link
Copy Markdown
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)

yuzefovich
yuzefovich approved these changes Sep 21, 2022
View reviewed changes
Copy link
Copy Markdown
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.

@DrewKimball
opt: run sqlsmith roachtest queries with EXPLAIN
3e23a24 
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
DrewKimball
DrewKimball commented Sep 21, 2022
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Collaborator Author

DrewKimball commented Sep 21, 2022 •
edited
Loading

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
Copy Markdown
Collaborator Author

DrewKimball commented Sep 21, 2022

bors r+

@DrewKimball DrewKimball mentioned this pull request Sep 21, 2022
Closed
@craig
Copy link
Copy Markdown
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

Reviewers

@yuzefovich yuzefovich yuzefovich approved these changes

@michae2 michae2 michae2 approved these changes

@mgartner mgartner Awaiting requested review from mgartner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@DrewKimball @cockroach-teamcity @yuzefovich @michae2