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

data race on (*PersistedSQLStats).Flush() #97488

Closed
maryliag opened this issue Feb 22, 2023 · 1 comment
Closed

data race on (*PersistedSQLStats).Flush() #97488

maryliag opened this issue Feb 22, 2023 · 1 comment
Assignees
@maryliag
Labels
A-sql-observability Related to observability of the SQL layer C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. C-test-failure Broken test (automatically or manually discovered).

Comments

@maryliag
Copy link
Contributor

maryliag commented Feb 22, 2023 •
edited by cockroach-jira-scripts
Loading

=== RUN TestSQLStatsPersistedLimitReached
test_log_scope.go:161: test logs captured to: /artifacts/tmp/_tmp/9a30d5271a65be6b3f2aa1433be9011f/logTestSQLStatsPersistedLimitReached2414865191
test_log_scope.go:79: use -show-logs to present logs inline

==================
WARNING: DATA RACE
Read at 0x00c002e6f908 by goroutine 84469:
github.com/cockroachdb/cockroach/pkg/sql/sqlstats/persistedsqlstats.(*PersistedSQLStats).Flush()
github.com/cockroachdb/cockroach/pkg/sql/sqlstats/persistedsqlstats/pkg/sql/sqlstats/persistedsqlstats/flush.go:39 +0x24d
pkg/sql/sqlstats/persistedsqlstats/persistedsqlstats_test_test.TestSQLStatsPersistedLimitReached()
pkg/sql/sqlstats/persistedsqlstats/persistedsqlstats_test_test/pkg/sql/sqlstats/persistedsqlstats/flush_test.go:506 +0x72c
github.com/cockroachdb/cockroach/pkg/server.(*Server).PreStart()
github.com/cockroachdb/cockroach/pkg/server/server.go:1823 +0x505c
github.com/cockroachdb/cockroach/pkg/server.(*Server).Start()
github.com/cockroachdb/cockroach/pkg/server/server.go:1235 +0x44
github.com/cockroachdb/cockroach/pkg/server.(*TestServer).Start()
github.com/cockroachdb/cockroach/pkg/server/testserver.go:601 +0x6c
github.com/cockroachdb/cockroach/pkg/testutils/serverutils.StartServer()
github.com/cockroachdb/cockroach/pkg/testutils/serverutils/test_server_shim.go:318 +0x1b1
pkg/sql/sqlstats/persistedsqlstats/persistedsqlstats_test_test.TestSQLStatsPersistedLimitReached()
pkg/sql/sqlstats/persistedsqlstats/persistedsqlstats_test_test/pkg/sql/sqlstats/persistedsqlstats/flush_test.go:469 +0x425
testing.tRunner()
GOROOT/src/testing/testing.go:1446 +0x216
testing.(*T).Run.func1()
GOROOT/src/testing/testing.go:1493 +0x47
Previous write at 0x00c002e6f908 by goroutine 84918:
github.com/cockroachdb/cockroach/pkg/sql/sqlstats/persistedsqlstats.(*PersistedSQLStats).Flush()
github.com/cockroachdb/cockroach/pkg/sql/sqlstats/persistedsqlstats/pkg/sql/sqlstats/persistedsqlstats/flush.go:68 +0x386
github.com/cockroachdb/cockroach/pkg/sql/sqlstats/persistedsqlstats.(*PersistedSQLStats).startSQLStatsFlushLoop.func1()
github.com/cockroachdb/cockroach/pkg/sql/sqlstats/persistedsqlstats/pkg/sql/sqlstats/persistedsqlstats/provider.go:149 +0x3f1
github.com/cockroachdb/cockroach/pkg/util/stop.(*Stopper).RunAsyncTaskEx.func2()
github.com/cockroachdb/cockroach/pkg/util/stop/stopper.go:470 +0x1f6
Goroutine 84469 (running) created at:
testing.(*T).Run()
GOROOT/src/testing/testing.go:1493 +0x75d
testing.runTests.func1()
GOROOT/src/testing/testing.go:1846 +0x99
testing.tRunner()
GOROOT/src/testing/testing.go:1446 +0x216
testing.runTests()
GOROOT/src/testing/testing.go:1844 +0x7ec
testing.(*M).Run()
GOROOT/src/testing/testing.go:1726 +0xa84
pkg/sql/sqlstats/persistedsqlstats/persistedsqlstats_test_test.TestMain()
pkg/sql/sqlstats/persistedsqlstats/persistedsqlstats_test_test/pkg/sql/sqlstats/persistedsqlstats/main_test.go:28 +0x186
main.main()
main/bazel-out/k8-fastbuild/bin/pkg/sql/sqlstats/persistedsqlstats/persistedsqlstats_test_/testmain.go:139 +0x748
Goroutine 84918 (running) created at:
github.com/cockroachdb/cockroach/pkg/util/stop.(*Stopper).RunAsyncTaskEx()
github.com/cockroachdb/cockroach/pkg/util/stop/stopper.go:461 +0x619
github.com/cockroachdb/cockroach/pkg/util/stop.(*Stopper).RunAsyncTask()
github.com/cockroachdb/cockroach/pkg/util/stop/stopper.go:332 +0x150
github.com/cockroachdb/cockroach/pkg/sql/sqlstats/persistedsqlstats.(*PersistedSQLStats).startSQLStatsFlushLoop()
github.com/cockroachdb/cockroach/pkg/sql/sqlstats/persistedsqlstats/pkg/sql/sqlstats/persistedsqlstats/provider.go:115 +0x26
github.com/cockroachdb/cockroach/pkg/sql/sqlstats/persistedsqlstats.(*PersistedSQLStats).Start()
github.com/cockroachdb/cockroach/pkg/sql/sqlstats/persistedsqlstats/pkg/sql/sqlstats/persistedsqlstats/provider.go:102 +0x53
github.com/cockroachdb/cockroach/pkg/sql.(*Server).Start()
github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:559 +0xc9
github.com/cockroachdb/cockroach/pkg/sql/pgwire.(*Server).Start()
github.com/cockroachdb/cockroach/pkg/sql/pgwire/server.go:377 +0xfcf
github.com/cockroachdb/cockroach/pkg/server.(*SQLServer).preStart()
github.com/cockroachdb/cockroach/pkg/server/server_sql.go:1446 +0xf65
github.com/cockroachdb/cockroach/pkg/sql.GetLocalityRegionEnumPhysicalRepresentation()
github.com/cockroachdb/cockroach/pkg/sql/region_util.go:1481 +0x21b
github.com/cockroachdb/cockroach/pkg/server.(*SQLServer).preStart()
github.com/cockroachdb/cockroach/pkg/server/server_sql.go:1367 +0x193
github.com/cockroachdb/cockroach/pkg/server.(*Server).PreStart()
github.com/cockroachdb/cockroach/pkg/server/server.go:1881 +0x5684
github.com/cockroachdb/cockroach/pkg/server.(*Node).writeNodeStatus()
github.com/cockroachdb/cockroach/pkg/server/node.go:1020 +0xf7
github.com/cockroachdb/cockroach/pkg/server.(*Node).startWriteNodeStatus()
github.com/cockroachdb/cockroach/pkg/server/node.go:980 +0xae
github.com/cockroachdb/cockroach/pkg/server.(*Server).PreStart()
github.com/cockroachdb/cockroach/pkg/server/server.go:1823 +0x505c
github.com/cockroachdb/cockroach/pkg/server.(*Server).Start()
github.com/cockroachdb/cockroach/pkg/server/server.go:1235 +0x44
github.com/cockroachdb/cockroach/pkg/server.(*TestServer).Start()
github.com/cockroachdb/cockroach/pkg/server/testserver.go:601 +0x6c
github.com/cockroachdb/cockroach/pkg/testutils/serverutils.StartServer()
github.com/cockroachdb/cockroach/pkg/testutils/serverutils/test_server_shim.go:318 +0x1b1
pkg/sql/sqlstats/persistedsqlstats/persistedsqlstats_test_test.TestSQLStatsPersistedLimitReached()
pkg/sql/sqlstats/persistedsqlstats/persistedsqlstats_test_test/pkg/sql/sqlstats/persistedsqlstats/flush_test.go:469 +0x425
testing.tRunner()
GOROOT/src/testing/testing.go:1446 +0x216
testing.(*T).Run.func1()
GOROOT/src/testing/testing.go:1493 +0x47

Jira issue: CRDB-24724
@maryliag maryliag added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-sql-observability Related to observability of the SQL layer T-sql-observability labels Feb 22, 2023
maryliag added a commit to maryliag/cockroach that referenced this issue Feb 22, 2023
@maryliag
test: remove no longer valid test
770d25b 
This test was checking for a behaviour that no longer
is the case. With the limitation of how much data we
can flush, we might not have the same fingerprints as
expected on this test.
Removing the test completly.

Fixes cockroachdb#97458

Also skip test TestSQLStatsPersistedLimitReached while
investigating.
Informs cockroachdb#97488

Release note: None
@maryliag maryliag mentioned this issue Feb 22, 2023
Merged
craig bot pushed a commit that referenced this issue Feb 22, 2023
@rharding6373 @maryliag
Merge #97274 #97472
6b6a8fb 
97274: sql: allow replace udf with implicit return type r=rharding6373 a=rharding6373

Before this change, we would return a "cannot change return type of existing function" when the user tried to replace a UDF with a function referencing the same UDT return type if the UDT had been altered since the original UDF was created.

This PR relaxes this constraint to match postgres behavior. Since the optbuilder already checks that the UDT return type is consistent with the columns returned by the UDF body, all we needed to do here was update the UDF descriptor.

Fixes: #94124

Epic: none
Release note: Fixes an error when calling `CREATE OR REPLACE FUNCTION` with a user-defined return type if the UDT was modified since the original UDF was created. Now the command will succeed as long as the function body returns the correct output matching the modified UDT.

97472: test: remove no longer valid test r=maryliag a=maryliag

This test was checking for a behaviour that no longer is the case. With the limitation of how much data we can flush, we might not have the same fingerprints as expected on this test.
Removing the test completely.

Fixes #97458

Also skip test TestSQLStatsPersistedLimitReached while investigating.
Informs #97488

Release note: None

Co-authored-by: rharding6373 <[email protected]>
Co-authored-by: maryliag <[email protected]>
maryliag added a commit to maryliag/cockroach that referenced this issue Feb 22, 2023
@maryliag
test: remove no longer valid test
6ab4087 
This test was checking for a behaviour that no longer
is the case. With the limitation of how much data we
can flush, we might not have the same fingerprints as
expected on this test.
Removing the test completly.

Fixes cockroachdb#97458

Also skip test TestSQLStatsPersistedLimitReached while
investigating.
Informs cockroachdb#97488

Release note: None
@maryliag maryliag mentioned this issue Feb 22, 2023
Merged
@maryliag maryliag self-assigned this Feb 24, 2023
zachlite added a commit to zachlite/cockroach that referenced this issue Jun 28, 2023
@zachlite
persistedsqlstats: skip TestSQLStatsPersistedLimitReached under stress
bc70750 
TestSQLStatsPersistedLimitReached succeeds under 'normal' testing conditions.
We can and should get this test coverage when we can.

Informs cockroachdb#97488
Epic: none
Release note: None
@zachlite zachlite mentioned this issue Jun 28, 2023
Merged
craig bot pushed a commit that referenced this issue Jun 29, 2023
@fqazi @renatolabs
@xinhaoz @zachlite @nvanbenschoten
Merge #104350 #105454 #105765 #105770 #105799
2b66f95 
104350: sql/schemachanger: support for create sequence inside the declarative schema changer r=fqazi a=fqazi

This patch implements CREATE SEQUENCE in the declarative schema changer. The first three commits can be ignored and are included in (#104348, which should be reviewed and merged first). This patch will:

- Skip validation/backfill for descriptors in an added state
- Fixes prefix resolution creating new objects, where two-part names were not handled properly before
- Adds support for CREATE sequence in the declarative schema changer

Fixes: #104351

105454: roachtest: update `mixedversion` to always use secure clusters r=srosenberg a=renatolabs

Secure clusters are closer to production deployments and also allow
us to tests features that we couldn't before, like creating new users
with passwords during a test, and then performing SQL operations with
those users.

In the process of getting this to work, there were a few bugs that needed
to be fixed (first commit), and the `cluster` interface needed a small update
as well (second commit).

Epic: CRDB-19321

Release note: None

105765: ui: fix db page Node/Regions column rendering r=xinhaoz a=xinhaoz

Previously, the db page was not updating its columns if the
`showNodeRegionsColumn` prop changed. The db details page was
also not filtering out the regions column when desired.

Epic: none

Release note (bug fix): node/regions columns in db and db details
page should properly render. This column is hidden for tenants and
otherwise is shown for clusters with > 1 node.

105770: persistedsqlstats: skip TestSQLStatsPersistedLimitReached under stress r=zachlite a=zachlite

TestSQLStatsPersistedLimitReached succeeds under 'normal' testing conditions. We can and should get this test coverage when we can.

Informs #97488
Epic: none
Release note: None

105799: kv: expose env var to configure raft entry cache size r=erikgrinaker a=nvanbenschoten

Informs #98666.

This commit introduces a new `COCKROACH_RAFT_ENTRY_CACHE_SIZE` which can be used to configure the size of the raft entry cache. The default value is 16 MiB.

Release note: None

Co-authored-by: Faizan Qazi <[email protected]>
Co-authored-by: Renato Costa <[email protected]>
Co-authored-by: Xin Hao Zhang <[email protected]>
Co-authored-by: zachlite <[email protected]>
Co-authored-by: Nathan VanBenschoten <[email protected]>
@blathers-crl blathers-crl bot mentioned this issue Jun 29, 2023
Merged
blathers-crl bot pushed a commit that referenced this issue Jun 29, 2023
@zachlite
persistedsqlstats: skip TestSQLStatsPersistedLimitReached under stress
de3771f 
TestSQLStatsPersistedLimitReached succeeds under 'normal' testing conditions.
We can and should get this test coverage when we can.

Informs #97488
Epic: none
Release note: None
zachlite added a commit to zachlite/cockroach that referenced this issue Jun 30, 2023
@zachlite
persistedsqlstats: skip TestSQLStatsPersistedLimitReached
0e46af9 
Flakes in Bazel essential CI.

Informs cockroachdb#105846, cockroachdb#97488
Epic: none
Release note: None
@zachlite zachlite mentioned this issue Jun 30, 2023
Merged
@rafiss rafiss added the C-test-failure Broken test (automatically or manually discovered). label Jun 30, 2023
craig bot pushed a commit that referenced this issue Jun 30, 2023
@chengxiong-ruan @zachlite
Merge #105758 #105893
947a636 
105758: sql: skip multiregional tests again r=chengxiong-ruan a=chengxiong-ruan

Unfortunately, it's still flaky:
#105346 (comment)

Informs: #98020

Release note: None

105893: persistedsqlstats: skip TestSQLStatsPersistedLimitReached r=zachlite a=zachlite

Flakes in Bazel essential CI.
Reverts #105770

Informs #105846, #97488
Epic: none
Release note: None

Co-authored-by: Chengxiong Ruan <[email protected]>
Co-authored-by: zachlite <[email protected]>
@blathers-crl blathers-crl bot mentioned this issue Jun 30, 2023
Merged
blathers-crl bot pushed a commit that referenced this issue Jun 30, 2023
@zachlite
persistedsqlstats: skip TestSQLStatsPersistedLimitReached
b9747c9 
Flakes in Bazel essential CI.

Informs #105846, #97488
Epic: none
Release note: None
zachlite added a commit to zachlite/cockroach that referenced this issue Jun 30, 2023
@zachlite
persistedsqlstats: de-flake TestSQLStatsPersistedLimitReached
9ba1daa 
TestSQLStatsPersistedLimitReached would previously flake
because it did not consider background SQL activity happening
in the cluster.

This commit re-writes the test to only make assertions that are
not dependent on background cluster activity.

Resolves cockroachdb#105846, cockroachdb#97488
Epic: none
Release note: None
@zachlite zachlite mentioned this issue Jun 30, 2023
Merged
zachlite added a commit to zachlite/cockroach that referenced this issue Jun 30, 2023
@zachlite
persistedsqlstats: de-flake TestSQLStatsPersistedLimitReached
dc4da0a 
TestSQLStatsPersistedLimitReached would previously flake
because it did not consider background SQL activity happening
in the cluster.

This commit re-writes the test to only make assertions that are
not dependent on background cluster activity.

Resolves cockroachdb#105846, cockroachdb#97488
Epic: none
Release note: None
zachlite added a commit to zachlite/cockroach that referenced this issue Jun 30, 2023
@zachlite
persistedsqlstats: de-flake TestSQLStatsPersistedLimitReached
f93c423 
TestSQLStatsPersistedLimitReached would previously flake
because it did not consider background SQL activity happening
in the cluster.

This commit re-writes the test to only make assertions that are
not dependent on background cluster activity.

Resolves cockroachdb#105846, cockroachdb#97488
Epic: none
Release note: None
zachlite added a commit to zachlite/cockroach that referenced this issue Jun 30, 2023
@zachlite
persistedsqlstats: de-flake TestSQLStatsPersistedLimitReached
a9a8a57 
TestSQLStatsPersistedLimitReached would previously flake
because it did not consider background SQL activity happening
in the cluster.

This commit re-writes the test to only make assertions that are
not dependent on background cluster activity.

Resolves cockroachdb#105846, cockroachdb#97488
Epic: none
Release note: None
craig bot pushed a commit that referenced this issue Jun 30, 2023
@zachlite
Merge #105917
9705c25 
105917: persistedsqlstats: de-flake TestSQLStatsPersistedLimitReached r=zachlite a=zachlite

TestSQLStatsPersistedLimitReached would previously flake because it did not consider background SQL activity happening in the cluster.

This commit re-writes the test to only make assertions that are not dependent on background cluster activity.

Resolves #105846, #97488
Epic: none
Release note: None

Co-authored-by: zachlite <[email protected]>
@rafiss
Copy link
Collaborator

rafiss commented Jun 30, 2023

closed by #105917

@rafiss rafiss closed this as completed Jun 30, 2023
blathers-crl bot pushed a commit that referenced this issue Jun 30, 2023
@zachlite
persistedsqlstats: de-flake TestSQLStatsPersistedLimitReached
b1301cd 
TestSQLStatsPersistedLimitReached would previously flake
because it did not consider background SQL activity happening
in the cluster.

This commit re-writes the test to only make assertions that are
not dependent on background cluster activity.

Resolves #105846, #97488
Epic: none
Release note: None
@blathers-crl blathers-crl bot mentioned this issue Jun 30, 2023
Merged
THardy98 pushed a commit to THardy98/cockroach that referenced this issue Jul 28, 2023
@zachlite
persistedsqlstats: de-flake TestSQLStatsPersistedLimitReached
2962a31 
TestSQLStatsPersistedLimitReached would previously flake
because it did not consider background SQL activity happening
in the cluster.

This commit re-writes the test to only make assertions that are
not dependent on background cluster activity.

Resolves cockroachdb#105846, cockroachdb#97488
Epic: none
Release note: None
@THardy98 THardy98 mentioned this issue Jul 28, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@maryliag maryliag

Labels
A-sql-observability Related to observability of the SQL layer C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. C-test-failure Broken test (automatically or manually discovered).
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@maryliag @rafiss