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.

Sign up for GitHub

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

Jump to bottom

sql: logic test can throw nil-pointer #88398

Closed
DrewKimball opened this issue Sep 21, 2022 · 2 comments · Fixed by #90802
Closed

sql: logic test can throw nil-pointer #88398

DrewKimball opened this issue Sep 21, 2022 · 2 comments · Fixed by #90802
Assignees
@msirek
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-queries SQL Queries Team

Comments

@DrewKimball
Copy link
Collaborator

DrewKimball commented Sep 21, 2022 •
edited by cockroach-jira-scripts
Loading

Describe the problem

There was a test failure in the CI build for #88333 that looked like this:

=== 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)

The problem is likely that SystemConfig.PurgeZoneConfigCache is called unconditionally on the result of SystemConfigProvider.GetSystemConfig, even though the result of GetSystemConfig can be nil.

Expected behavior
Either the nil-receiver case should be handled, or PurgeZoneConfigCache shouldn't be called when SystemConfig is nil.

Additional context

This panic should be testing-only, since it's only called in TestCluster and TestLogic.

Jira issue: CRDB-19801
@DrewKimball DrewKimball added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Sep 21, 2022
@blathers-crl blathers-crl bot added the T-sql-queries SQL Queries Team label Sep 21, 2022
@rytaft
Copy link
Collaborator

rytaft commented Oct 27, 2022

@msirek are you planning to work on this? If not, please unassigned yourself. thanks!

@msirek msirek mentioned this issue Oct 27, 2022
Merged
@msirek
Copy link
Contributor

msirek commented Oct 27, 2022

@rytaft Thanks for bringing this to my attention. I've opened a PR.

craig bot pushed a commit that referenced this issue Oct 27, 2022
@rafiss @irfansharif
@yuzefovich @rail
Merge #90780 #90788 #90792 #90802 #90804
6fba541 
90780: rttanalysis: add benchmark for hasura's introspection query r=rafiss a=rafiss

informs #88885

This query exposes an issue with how we construct filters for certain types of joins. It can be used to verify future enhancements.

Release note: None

90788: kvserver: improve help text for `admission.io.overload` r=irfansharif a=irfansharif

Since this is text that's available in any running binary (`SHOW ALL CLUSTER SETTINGS`), we shouldn't expect readers to have the source code checked out, which makes our referencing of specific file names less than useful. See #87656 (review).

Release note: None
Release justification: No behavioural change, only changing user-visible help text.

90792: ui: address an old TODO r=yuzefovich a=yuzefovich

This commit addresses an old TODO to simplify the check for when we warn about full scans.

Epic: None

Release note: None

90802: logictest: check for nil SystemConfig before purging zone config cache r=yuzefovich a=msirek

Fixes #88398

This fixes a panic which can occur duing cluster startup in a logic test or during retry of a test case in a logic test when an attempt is made to purge the zone config cache and no SystemConfig is available.

Release note: None

90804: release: S3 redirects should use abs path r=celiala a=rail

Previously, S3 "latest" redirects used relative path, while the AWS API expects the redirects to start with "/", "http://", or "https://".

This patch adds a slash to the key name for all redirect calls. The GCS implementation already strips the leading slash and should handle it properly.

Release note: None
Epic: None

Co-authored-by: Rafi Shamim <[email protected]>
Co-authored-by: irfan sharif <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: Mark Sirek <[email protected]>
Co-authored-by: Rail Aliiev <[email protected]>
@craig craig bot closed this as completed in 3019488 Oct 27, 2022
msirek pushed a commit to msirek/cockroach that referenced this issue Jun 29, 2023
logictest: check for nil SystemConfig before purging zone config cache
eb1c431 
Fixes cockroachdb#88398

This fixes a panic which can occur duing cluster startup in a logic test
or during retry of a test case in a logic test when an attempt is made
to purge the zone config cache and no SystemConfig is available.

Release note: None
@mgartner mgartner moved this to Done in SQL Queries Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@msirek msirek

Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-queries SQL Queries Team
Projects
SQL Queries
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

logictest: check for nil SystemConfig before purging zone config cache msirek/cockroach
3 participants
@rytaft @msirek @DrewKimball