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

server: data race in VirtualizationWithContext #61091

Closed
RaduBerinde opened this issue Feb 24, 2021 · 0 comments · Fixed by #61100
Closed

server: data race in VirtualizationWithContext #61091

RaduBerinde opened this issue Feb 24, 2021 · 0 comments · Fixed by #61100
Assignees
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@RaduBerinde
Copy link
Member

Test failure: https://teamcity.cockroachdb.com//viewLog.html?buildId=2714781&buildTypeId=Cockroach_MergedExtendedCi

Race from full_output.txt:

WARNING: DATA RACE
Write at 0x00000a27b408 by goroutine 603:
  github.com/shirou/gopsutil/internal/common.VirtualizationWithContext()
      /go/src/github.com/cockroachdb/cockroach/vendor/github.com/shirou/gopsutil/internal/common/common_linux.go:243 +0x955
  github.com/shirou/gopsutil/host.VirtualizationWithContext()
      /go/src/github.com/cockroachdb/cockroach/vendor/github.com/shirou/gopsutil/host/host_linux.go:365 +0x5a
  github.com/shirou/gopsutil/host.Virtualization()
      /go/src/github.com/cockroachdb/cockroach/vendor/github.com/shirou/gopsutil/host/host.go:141 +0x97
  github.com/cockroachdb/cockroach/pkg/server/diagnostics.populateHardwareInfo()
      /go/src/github.com/cockroachdb/cockroach/pkg/server/diagnostics/reporter.go:350 +0x77
  github.com/cockroachdb/cockroach/pkg/server/diagnostics.(*UpdateChecker).buildUpdatesURL()
      /go/src/github.com/cockroachdb/cockroach/pkg/server/diagnostics/update_checker.go:194 +0x45a
  github.com/cockroachdb/cockroach/pkg/server/diagnostics.(*UpdateChecker).CheckForUpdates()
      /go/src/github.com/cockroachdb/cockroach/pkg/server/diagnostics/update_checker.go:106 +0x12f
  github.com/cockroachdb/cockroach/pkg/server/diagnostics.(*UpdateChecker).maybeCheckForUpdates()
      /go/src/github.com/cockroachdb/cockroach/pkg/server/diagnostics/update_checker.go:166 +0x111
  github.com/cockroachdb/cockroach/pkg/server/diagnostics.(*UpdateChecker).PeriodicallyCheckForUpdates.func1()
      /go/src/github.com/cockroachdb/cockroach/pkg/server/diagnostics/update_checker.go:85 +0x350
  github.com/cockroachdb/cockroach/pkg/util/stop.(*Stopper).RunAsyncTask.func1()
      /go/src/github.com/cockroachdb/cockroach/pkg/util/stop/stopper.go:351 +0x149

Previous write at 0x00000a27b408 by goroutine 230:
  github.com/shirou/gopsutil/internal/common.VirtualizationWithContext()
      /go/src/github.com/cockroachdb/cockroach/vendor/github.com/shirou/gopsutil/internal/common/common_linux.go:243 +0x955
  github.com/shirou/gopsutil/host.VirtualizationWithContext()
      /go/src/github.com/cockroachdb/cockroach/vendor/github.com/shirou/gopsutil/host/host_linux.go:365 +0x5a
  github.com/shirou/gopsutil/host.Virtualization()
      /go/src/github.com/cockroachdb/cockroach/vendor/github.com/shirou/gopsutil/host/host.go:141 +0x97
  github.com/cockroachdb/cockroach/pkg/server/diagnostics.populateHardwareInfo()
      /go/src/github.com/cockroachdb/cockroach/pkg/server/diagnostics/reporter.go:350 +0x77
  github.com/cockroachdb/cockroach/pkg/server/diagnostics.(*Reporter).populateEnvironment()
      /go/src/github.com/cockroachdb/cockroach/pkg/server/diagnostics/reporter.go:248 +0x424
  github.com/cockroachdb/cockroach/pkg/server/diagnostics.(*Reporter).CreateReport()
      /go/src/github.com/cockroachdb/cockroach/pkg/server/diagnostics/reporter.go:173 +0x21a
  github.com/cockroachdb/cockroach/pkg/server/diagnostics.(*Reporter).ReportDiagnostics()
      /go/src/github.com/cockroachdb/cockroach/pkg/server/diagnostics/reporter.go:128 +0x128
  github.com/cockroachdb/cockroach/pkg/server/diagnostics.(*Reporter).PeriodicallyReportDiagnostics.func1()
      /go/src/github.com/cockroachdb/cockroach/pkg/server/diagnostics/reporter.go:104 +0x552
  github.com/cockroachdb/cockroach/pkg/util/stop.(*Stopper).RunAsyncTask.func1()
      /go/src/github.com/cockroachdb/cockroach/pkg/util/stop/stopper.go:351 +0x149

Goroutine 603 (running) created at:
  github.com/cockroachdb/cockroach/pkg/util/stop.(*Stopper).RunAsyncTask()
      /go/src/github.com/cockroachdb/cockroach/pkg/util/stop/stopper.go:346 +0x107
  github.com/cockroachdb/cockroach/pkg/server/diagnostics.(*UpdateChecker).PeriodicallyCheckForUpdates()
      /go/src/github.com/cockroachdb/cockroach/pkg/server/diagnostics/update_checker.go:75 +0xf6
  github.com/cockroachdb/cockroach/pkg/server.(*Server).StartDiagnostics()
      /go/src/github.com/cockroachdb/cockroach/pkg/server/server.go:2364 +0x99
  github.com/cockroachdb/cockroach/pkg/cli.(*transientCluster).start()
      /go/src/github.com/cockroachdb/cockroach/pkg/cli/demo_cluster.go:299 +0x1976
  github.com/cockroachdb/cockroach/pkg/server.(*Server).PreStart()
      /go/src/github.com/cockroachdb/cockroach/pkg/server/server.go:1666 +0x447e
  github.com/cockroachdb/cockroach/pkg/server.(*Server).Start()
      /go/src/github.com/cockroachdb/cockroach/pkg/server/server.go:1079 +0x50
  github.com/cockroachdb/cockroach/pkg/server.(*TestServer).Start()
      /go/src/github.com/cockroachdb/cockroach/pkg/server/testserver.go:461 +0x8b5
  github.com/cockroachdb/cockroach/pkg/cli.(*transientCluster).start()
      /go/src/github.com/cockroachdb/cockroach/pkg/cli/demo_cluster.go:206 +0x8d0
  github.com/cockroachdb/cockroach/pkg/cli.runDemo()
      /go/src/github.com/cockroachdb/cockroach/pkg/cli/demo.go:279 +0x31c
  github.com/cockroachdb/cockroach/pkg/cli.init.4.func1()
      /go/src/github.com/cockroachdb/cockroach/pkg/cli/demo.go:55 +0x44
  github.com/cockroachdb/cockroach/pkg/cli.MaybeDecorateGRPCError.func1()
      /go/src/github.com/cockroachdb/cockroach/pkg/cli/error.go:194 +0x99
  github.com/spf13/cobra.(*Command).execute()
      /go/src/github.com/cockroachdb/cockroach/vendor/github.com/spf13/cobra/command.go:826 +0x535
  github.com/spf13/cobra.(*Command).ExecuteC()
      /go/src/github.com/cockroachdb/cockroach/vendor/github.com/spf13/cobra/command.go:914 +0x431
  github.com/spf13/cobra.(*Command).Execute()
      /go/src/github.com/cockroachdb/cockroach/vendor/github.com/spf13/cobra/command.go:864 +0x364
  github.com/cockroachdb/cockroach/pkg/cli.Run()
      /go/src/github.com/cockroachdb/cockroach/pkg/cli/cli.go:296 +0x343
  github.com/cockroachdb/cockroach/pkg/cli.cliTest.RunWithArgs.func1()
      /go/src/github.com/cockroachdb/cockroach/pkg/cli/cli_test.go:352 +0x380
  github.com/cockroachdb/cockroach/pkg/cli.cliTest.RunWithArgs()
      /go/src/github.com/cockroachdb/cockroach/pkg/cli/cli_test.go:353 +0xe4
  github.com/cockroachdb/cockroach/pkg/cli.Example_demo()
      /go/src/github.com/cockroachdb/cockroach/pkg/cli/cli_test.go:444 +0x127d
  testing.runExample()
      /usr/local/go/src/testing/run_example.go:62 +0x2b5
  testing.runExamples()
      /usr/local/go/src/testing/example.go:44 +0x228
  testing.(*M).Run()
      /usr/local/go/src/testing/testing.go:1346 +0x464
  github.com/cockroachdb/cockroach/pkg/cli_test.TestMain()
      /go/src/github.com/cockroachdb/cockroach/pkg/cli/main_test.go:34 +0xaf
  main.main()
      _testmain.go:219 +0x271

Goroutine 230 (running) created at:
  github.com/cockroachdb/cockroach/pkg/util/stop.(*Stopper).RunAsyncTask()
      /go/src/github.com/cockroachdb/cockroach/pkg/util/stop/stopper.go:346 +0x107
  github.com/cockroachdb/cockroach/pkg/server/diagnostics.(*Reporter).PeriodicallyReportDiagnostics()
      /go/src/github.com/cockroachdb/cockroach/pkg/server/diagnostics/reporter.go:93 +0xf6
  github.com/cockroachdb/cockroach/pkg/server.(*SQLServer).StartDiagnostics()
      /go/src/github.com/cockroachdb/cockroach/pkg/server/server_sql.go:898 +0x11a
  github.com/cockroachdb/cockroach/pkg/server.(*Server).StartDiagnostics()
      /go/src/github.com/cockroachdb/cockroach/pkg/server/server.go:2365 +0x9a
  github.com/cockroachdb/cockroach/pkg/cli.(*transientCluster).start()
      /go/src/github.com/cockroachdb/cockroach/pkg/cli/demo_cluster.go:299 +0x1976
  github.com/cockroachdb/cockroach/pkg/server.(*Server).PreStart()
      /go/src/github.com/cockroachdb/cockroach/pkg/server/server.go:1666 +0x447e
  github.com/cockroachdb/cockroach/pkg/server.(*Server).Start()
      /go/src/github.com/cockroachdb/cockroach/pkg/server/server.go:1079 +0x50
  github.com/cockroachdb/cockroach/pkg/server.(*TestServer).Start()
      /go/src/github.com/cockroachdb/cockroach/pkg/server/testserver.go:461 +0x8b5
  github.com/cockroachdb/cockroach/pkg/cli.(*transientCluster).start()
      /go/src/github.com/cockroachdb/cockroach/pkg/cli/demo_cluster.go:206 +0x8d0
  github.com/cockroachdb/cockroach/pkg/cli.runDemo()
      /go/src/github.com/cockroachdb/cockroach/pkg/cli/demo.go:279 +0x31c
  github.com/cockroachdb/cockroach/pkg/cli.init.4.func1()
      /go/src/github.com/cockroachdb/cockroach/pkg/cli/demo.go:55 +0x44
  github.com/cockroachdb/cockroach/pkg/cli.MaybeDecorateGRPCError.func1()
      /go/src/github.com/cockroachdb/cockroach/pkg/cli/error.go:194 +0x99
  github.com/spf13/cobra.(*Command).execute()
      /go/src/github.com/cockroachdb/cockroach/vendor/github.com/spf13/cobra/command.go:826 +0x535
  github.com/spf13/cobra.(*Command).ExecuteC()
      /go/src/github.com/cockroachdb/cockroach/vendor/github.com/spf13/cobra/command.go:914 +0x431
  github.com/spf13/cobra.(*Command).Execute()
      /go/src/github.com/cockroachdb/cockroach/vendor/github.com/spf13/cobra/command.go:864 +0x364
  github.com/cockroachdb/cockroach/pkg/cli.Run()
      /go/src/github.com/cockroachdb/cockroach/pkg/cli/cli.go:296 +0x343
  github.com/cockroachdb/cockroach/pkg/cli.cliTest.RunWithArgs.func1()
      /go/src/github.com/cockroachdb/cockroach/pkg/cli/cli_test.go:352 +0x380
  github.com/cockroachdb/cockroach/pkg/cli.cliTest.RunWithArgs()
      /go/src/github.com/cockroachdb/cockroach/pkg/cli/cli_test.go:353 +0xe4
  github.com/cockroachdb/cockroach/pkg/cli.Example_demo()
      /go/src/github.com/cockroachdb/cockroach/pkg/cli/cli_test.go:444 +0x127d
  testing.runExample()
      /usr/local/go/src/testing/run_example.go:62 +0x2b5
  testing.runExamples()
      /usr/local/go/src/testing/example.go:44 +0x228
  testing.(*M).Run()
      /usr/local/go/src/testing/testing.go:1346 +0x464
  github.com/cockroachdb/cockroach/pkg/cli_test.TestMain()
      /go/src/github.com/cockroachdb/cockroach/pkg/cli/main_test.go:34 +0xaf
  main.main()
      _testmain.go:219 +0x271
==================
@RaduBerinde RaduBerinde added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Feb 24, 2021
craig bot pushed a commit that referenced this issue Feb 25, 2021
57183: importccl,sql: support importing non-public schemas from pgdump r=adityamaru a=adityamaru

Previously, a PGDUMP import did not support any non-public schema
statements. Now that CRDB has user defined schemas, bundle format
IMPORTs need to be taught how to parse, create and cleanup schema
related PGDUMP operations.

Note this PR only adds support for `CREATE SCHEMA` and usage of the
schema in `CREATE TABLE/SEQUENCE` PGDUMP statements. `ALTER SCHEMA`
statements are ignored and support might be added in a follow up.

Release justification: low risk, high reward. Import PGDUMP does not support user
defined schemas.

Release note (sql change): IMPORT PGDUMP can now import dump files with
non-public schemas.

60922: sql,cli: add payloads_for_trace builtin r=knz a=angelapwen

Possibly the final step to #55733. Resolves #58608 😄 

Previously it was quite cumbersome to view all payloads for a given
trace: we needed to join on the `node_inflight_trace_spans` vtable
to filter for span IDs that match a trace ID, then apply the
`payloads_for_span()` builtin to each span ID. This patch adds
syntactic sugar to the above query.

Instead of

```
WITH spans AS (
  SELECT span_id
  FROM crdb_internal.node_inflight_trace_spans
  WHERE trace_id = $TRACE_ID)
) SELECT *
  FROM spans, LATERAL crdb_internal.payloads_for_span(spans.span_id);
```

we can now simply use:
```
crdb_internal.payloads_for_trace($TRACE_ID);
```

and achieve the same result. The patch also adds all payloads for all
long-running spans to the `crdb_internal.node_inflight_trace_spans`
table of the debug.zip file.

Release note (sql change): Add `payloads_for_trace()` builtin so that
all payloads attached to all spans for a given trace ID will be
displayed, utilizing the `crdb_internal.payloads_for_span()`
builtin under the hood. All payloads for long-running spans are also
added to debug.zip in the `crdb_internal.node_inflight_trace_spans`
table dump.

Co-authored-by: Tobias Grieger <[email protected]>

Release justification: This patch is safe for release because it
adds syntactic sugar to an internal observability feature.

61094: kvserver: re-add spuriously removed nil check in `relocateOne` r=aayushshah15 a=aayushshah15

bce8317 introduced a bug by spuriously
removing a nil check over the result of a call to
`allocateTargetFromList`. This commit re-adds the check.

The bug could cause a panic when `AdminRelocateRange` was called by the
`StoreRebalancer` or the `mergeQueue` if one (or more) of the stores
that are supposed to receive a replica for a range become unfit for
receiving the replica (due to balancing reasons / or shifting
constraints) _between when rebalancing decision is made and when it's
executed_.

Resolves #60812

Release justification: fixes bug that causes a panic
Release note: None

61097: opt: use computed columns to improve FDs and remove uniqueness checks r=rytaft a=rytaft

**opt: use computed columns to build functional dependencies**

This commit updates `MakeTableFuncDep` so that it adds equivalencies
or synthesized columns to the table FDs for each of the computed
columns available in the metadata. This will be necessary to support
removing uniqueness checks in some cases in a future commit.

Release justification: This commit is a low risk, high benefit change
to existing functionality.

Release note (performance improvement): The optimizer now infers
additional functional dependencies based on computed columns in tables.
This may enable additional optimizations and lead to better query plans.

**opt: remove uniqueness checks when uniqueness inferred through FDs**

This commit removes uniqueness checks for columns that can be
inferred to be unique through functional dependencies. This is
relevant in particular for `REGIONAL BY ROW` tables with a computed
region column that depends on the primary key. In this case,
uniqueness checks are never needed on the primary key, since
uniqueness is already guaranteed by the primary index.

Fixes #57720

Release justification: This commit is a low-risk, high benefit
update to new functionality.

Release note (performance improvement): Removed uniqueness checks
on the primary key for REGIONAL BY ROW tables with a computed
region column that is a function of the primary key columns.
Uniqueness checks are not necessary in this case since uniqueness
can be suitably guaranteed by the primary index. Removing these
checks improves performance of INSERT, UPDATE, and UPSERT
statements.

61100: diagnostics: lock while populating hardware information r=andy-kimball a=andy-kimball

The shirou/gopsutil/host library that we use to gather hardware information
during diagnostics reporting is not multi-thread safe. As one example, it
lazily initializes a global map the first time the Virtualization function
is called, but takes no lock while doing so. Work around this limitation by
taking our own lock.

This code never triggered race conditions before, but is doing so after recent
changes I made to the diagnostics reporting code. Previously, we were using a
single background goroutine to do both diagnostics reporting and checking for
updates. Now we are doing each of those on different goroutines, which triggers
race detection.

Fixes #61091

Release justification: fixes for high-priority or high-severity bugs in existing
functionality
Release note: None

61105: builtins: implement ST_MakePoint and ST_MakePointM r=otan,rafiss a=andyyang890

This patch implements the geometry builtins `ST_MakePoint`
and `ST_MakePointM`.

Resolves #60857.
Resolves #60858.
Resolves #60859.

Release justification: low-risk update to new functionality
Release note (sql change): The geometry builtins `ST_MakePoint`
and `ST_MakePointM` have been implemented and provide a mechanism
for easily creating new points.

Co-authored-by: Aditya Maru <[email protected]>
Co-authored-by: angelapwen <[email protected]>
Co-authored-by: Aayush Shah <[email protected]>
Co-authored-by: Rebecca Taft <[email protected]>
Co-authored-by: Andrew Kimball <[email protected]>
Co-authored-by: Andy Yang <[email protected]>
@craig craig bot closed this as completed in 1aee317 Feb 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants