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

sql: data race in schema changer / plan visitor #88628

Closed
jordanlewis opened this issue Sep 23, 2022 · 1 comment · Fixed by #88670
Closed

sql: data race in schema changer / plan visitor #88628

jordanlewis opened this issue Sep 23, 2022 · 1 comment · Fixed by #88670
Labels
branch-release-22.2 Used to mark GA and release blockers, technical advisories, and bugs for 22.2 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. GA-blocker T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@jordanlewis
Copy link
Member

jordanlewis commented Sep 23, 2022

Seen in unrelated PR (#86968)

WARNING: DATA RACE
Read at 0x00c000ab4988 by goroutine 64179:
  github.com/cockroachdb/cockroach/pkg/sql/schemachanger/rel.(*typedValue).inlineValue()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/schemachanger/rel/query_data.go:79 +0x64
  github.com/cockroachdb/cockroach/pkg/sql/schemachanger/rel.(*evalContext).iterateNext()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/schemachanger/rel/query_eval.go:114 +0x4b2
  github.com/cockroachdb/cockroach/pkg/sql/schemachanger/rel.(*evalContext).Iterate()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/schemachanger/rel/query_eval.go:71 +0x204
  github.com/cockroachdb/cockroach/pkg/sql/schemachanger/rel.(*Query).Iterate()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/schemachanger/rel/query.go:99 +0xc7
  github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scplan/internal/rules.ApplyDepRules()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scplan/internal/rules/registry.go:34 +0x345
  github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scplan.buildGraph()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scplan/plan.go:131 +0xb5
  github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scplan.makePlan()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scplan/plan.go:102 +0x177
  github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scplan.MakePlan()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scplan/plan.go:82 +0x124
  github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scrun.runTransactionPhase()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scrun/scrun.go:68 +0x1a7
  github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scrun.RunStatementPhase()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scrun/scrun.go:42 +0xb6d
  github.com/cockroachdb/cockroach/pkg/sql.(*schemaChangePlanNode).startExec()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/schema_change_plan_node.go:225 +0xab2
  github.com/cockroachdb/cockroach/pkg/sql.startExec.func2()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/plan.go:517 +0x186
  github.com/cockroachdb/cockroach/pkg/sql.(*planVisitor).visitInternal.func1()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/walk.go:112 +0xa9
  runtime.deferreturn()
      /usr/local/go/src/runtime/panic.go:476 +0x32
  github.com/cockroachdb/cockroach/pkg/sql.(*planVisitor).visit()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/walk.go:79 +0x135
  github.com/cockroachdb/cockroach/pkg/sql.walkPlan()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/walk.go:43 +0x165
  github.com/cockroachdb/cockroach/pkg/sql.startExec()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/plan.go:520 +0x180
  github.com/cockroachdb/cockroach/pkg/sql.(*planNodeToRowSource).Start()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/plan_node_to_row_source.go:147 +0x15c
  github.com/cockroachdb/cockroach/pkg/sql/colflow.(*FlowCoordinator).Start.func1()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/colflow/flow_coordinator.go:122 +0x74
  github.com/cockroachdb/cockroach/pkg/sql/colexecerror.CatchVectorizedRuntimeError()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/colexecerror/error.go:92 +0x75
  github.com/cockroachdb/cockroach/pkg/sql/colflow.(*FlowCoordinator).Start()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/colflow/flow_coordinator.go:121 +0x88
  github.com/cockroachdb/cockroach/pkg/sql/execinfra.(*ProcessorBaseNoHelper).Run()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/execinfra/processorsbase.go:716 +0x79
  github.com/cockroachdb/cockroach/pkg/sql/colflow.(*FlowCoordinator).Run()
      <autogenerated>:1 +0x4b
  github.com/cockroachdb/cockroach/pkg/sql/flowinfra.(*FlowBase).Run()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/flowinfra/flow.go:472 +0x453
  github.com/cockroachdb/cockroach/pkg/sql/colflow.(*vectorizedFlow).Run()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/colflow/vectorized_flow.go:307 +0x38c
  github.com/cockroachdb/cockroach/pkg/sql.(*DistSQLPlanner).Run()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/distsql_running.go:695 +0x1017
  github.com/cockroachdb/cockroach/pkg/sql.(*DistSQLPlanner).PlanAndRun()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/distsql_running.go:1611 +0x3c4
  github.com/cockroachdb/cockroach/pkg/sql.(*DistSQLPlanner).PlanAndRunAll()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/distsql_running.go:1334 +0x2d1
  github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execWithDistSQLEngine()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:1535 +0xaf0
  github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).dispatchToExecutionEngine()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:1171 +0x1484
  github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execStmtInOpenState()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:681 +0x3209
  github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execStmt.func1()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:129 +0x18e
  github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execWithProfiling()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:2380 +0x50f
  github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execStmt()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:128 +0x6cd
  github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execCmd.func1()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1919 +0x790
  github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execCmd()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1923 +0x1cba
  github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).run()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1845 +0x424
  github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).run()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1845 +0x424
  github.com/cockroachdb/cockroach/pkg/sql.(*Server).ServeConn()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:827 +0x1be
  github.com/cockroachdb/cockroach/pkg/sql/pgwire.(*conn).processCommandsAsync.func1()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/pgwire/conn.go:728 +0x53b

Previous write at 0x00c000ab4988 by goroutine 64208:
  github.com/cockroachdb/cockroach/pkg/sql/schemachanger/rel.(*typedValue).inlineValue()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/schemachanger/rel/query_data.go:86 +0x168
  github.com/cockroachdb/cockroach/pkg/sql/schemachanger/rel.(*evalContext).iterateNext()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/schemachanger/rel/query_eval.go:114 +0x4b2
  github.com/cockroachdb/cockroach/pkg/sql/schemachanger/rel.(*evalContext).Iterate()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/schemachanger/rel/query_eval.go:71 +0x204
  github.com/cockroachdb/cockroach/pkg/sql/schemachanger/rel.(*Query).Iterate()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/schemachanger/rel/query.go:99 +0xc7
  github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scplan/internal/rules.ApplyDepRules()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scplan/internal/rules/registry.go:34 +0x345
  github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scplan.buildGraph()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scplan/plan.go:131 +0xb5
  github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scplan.makePlan()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scplan/plan.go:102 +0x177
  github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scplan.MakePlan()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scplan/plan.go:82 +0x124
  github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scrun.runTransactionPhase()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scrun/scrun.go:68 +0x1a7
  github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scrun.RunStatementPhase()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scrun/scrun.go:42 +0xb6d
  github.com/cockroachdb/cockroach/pkg/sql.(*schemaChangePlanNode).startExec()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/schema_change_plan_node.go:225 +0xab2
  github.com/cockroachdb/cockroach/pkg/sql.startExec.func2()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/plan.go:517 +0x186
  github.com/cockroachdb/cockroach/pkg/sql.(*planVisitor).visitInternal.func1()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/walk.go:112 +0xa9
  runtime.deferreturn()
      /usr/local/go/src/runtime/panic.go:476 +0x32
  github.com/cockroachdb/cockroach/pkg/sql.(*planVisitor).visit()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/walk.go:79 +0x135
  github.com/cockroachdb/cockroach/pkg/sql.walkPlan()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/walk.go:43 +0x165
  github.com/cockroachdb/cockroach/pkg/sql.startExec()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/plan.go:520 +0x180
  github.com/cockroachdb/cockroach/pkg/sql.(*planNodeToRowSource).Start()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/plan_node_to_row_source.go:147 +0x15c
  github.com/cockroachdb/cockroach/pkg/sql/colflow.(*FlowCoordinator).Start.func1()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/colflow/flow_coordinator.go:122 +0x74
  github.com/cockroachdb/cockroach/pkg/sql/colexecerror.CatchVectorizedRuntimeError()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/colexecerror/error.go:92 +0x75
  github.com/cockroachdb/cockroach/pkg/sql/colflow.(*FlowCoordinator).Start()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/colflow/flow_coordinator.go:121 +0x88
  github.com/cockroachdb/cockroach/pkg/sql/execinfra.(*ProcessorBaseNoHelper).Run()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/execinfra/processorsbase.go:716 +0x79
  github.com/cockroachdb/cockroach/pkg/sql/colflow.(*FlowCoordinator).Run()
      <autogenerated>:1 +0x4b
  github.com/cockroachdb/cockroach/pkg/sql/flowinfra.(*FlowBase).Run()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/flowinfra/flow.go:472 +0x453
  github.com/cockroachdb/cockroach/pkg/sql/colflow.(*vectorizedFlow).Run()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/colflow/vectorized_flow.go:307 +0x38c
  github.com/cockroachdb/cockroach/pkg/sql.(*DistSQLPlanner).Run()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/distsql_running.go:695 +0x1017
  github.com/cockroachdb/cockroach/pkg/sql.(*DistSQLPlanner).PlanAndRun()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/distsql_running.go:1611 +0x3c4
  github.com/cockroachdb/cockroach/pkg/sql.(*DistSQLPlanner).PlanAndRunAll()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/distsql_running.go:1334 +0x2d1
  github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execWithDistSQLEngine()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:1535 +0xaf0
  github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).dispatchToExecutionEngine()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:1171 +0x1484
  github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execStmtInOpenState()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:681 +0x3209
  github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execStmt.func1()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:129 +0x18e
  github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execWithProfiling()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:2380 +0x50f
  github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execStmt()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:128 +0x6cd
  github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execCmd.func1()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1919 +0x790
  github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execCmd()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1923 +0x1cba
  github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).run()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1845 +0x424
  github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).run()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1845 +0x424
  github.com/cockroachdb/cockroach/pkg/sql.(*Server).ServeConn()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:827 +0x1be
  github.com/cockroachdb/cockroach/pkg/sql/pgwire.(*conn).processCommandsAsync.func1()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/pgwire/conn.go:728 +0x53b

Jira issue: CRDB-19987

@jordanlewis jordanlewis added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Sep 23, 2022
@blathers-crl blathers-crl bot added the T-sql-schema-deprecated Use T-sql-foundations instead label Sep 23, 2022
@blathers-crl
Copy link

blathers-crl bot commented Sep 23, 2022

Hi @ajwerner, please add branch-* labels to identify which branch(es) this release-blocker affects.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

ajwerner added a commit to ajwerner/cockroach that referenced this issue Sep 26, 2022
The fundamental race here is that while the slots themselves were being copied
by value, the "any" clauses which are a slice were not. The second bug here is
that the "inline" values were not being properly reset. That bug could lead to
problems when the query was run again in the context of a different element
set. We need to reset those inline values too.

Fixes cockroachdb#88628

Release note: None
@ajwerner ajwerner added the branch-release-22.2 Used to mark GA and release blockers, technical advisories, and bugs for 22.2 label Sep 26, 2022
ajwerner added a commit to ajwerner/cockroach that referenced this issue Sep 26, 2022
The fundamental race here is that while the slots themselves were being copied
by value, the "any" clauses which are a slice were not. The second bug here is
that the "inline" values were not being properly reset. That bug could lead to
problems when the query was run again in the context of a different element
set. We need to reset those inline values too.

Fixes cockroachdb#88628

Release note: None
ajwerner added a commit to ajwerner/cockroach that referenced this issue Sep 26, 2022
The fundamental race here is that while the slots themselves were being copied
by value, the "any" clauses which are a slice were not. The second bug here is
that the "inline" values were not being properly reset. That bug could lead to
problems when the query was run again in the context of a different element
set. We need to reset those inline values too.

Fixes cockroachdb#88628

Release note: None
craig bot pushed a commit that referenced this issue Sep 26, 2022
88252: storage: add gc benchmarks with multiple range tombstones r=erikgrinaker a=aliher1911

Previously we only had tests that verify GC performance with no range tombstones or with a single range tombstone under all the data. This commit extends the benchmark to have many tombstones interleaving data at different keys.

Release note: None

Fixes #87417

88295: sql: add assertion for txn passed to internal executor r=ajwerner a=ZhouXing19

If an internal executor is created with a txn binding to it, it should not be used to execute a statement with a nil txn.

Release note: None

88670: schemachanger/rel: fix race due to failure to clone constraint slots r=ajwerner a=ajwerner

The fundamental race here is that while the slots themselves were being copied by value, the "any" clauses which are a slice were not. The second bug here is that the "inline" values were not being properly reset. That bug could lead to problems when the query was run again in the context of a different element set. We need to reset those inline values too.

Fixes #88628

Release note: None

88716: roachprod: set default cluster settings when starting r=yuzefovich,srosenberg,smg260 a=renatolabs

In #88514, the cluster start logic was refactored to reuse the same code across `init` and `start`, fixing a bug in the former. However, the refactoring overlooked the fact that we previously always set the default cluster settings when there's more than one node in the cluster.

This fixes that by setting the default cluster settings in that case; one particularly important cluster setting is the license key, necessary for some roachtests.

Fixes #88660
Fixes #88665
Fixes #88666
Fixes #88710.

Release note: None

Co-authored-by: Oleg Afanasyev <oleg@cockroachlabs.com>
Co-authored-by: Jane Xing <zhouxing@uchicago.edu>
Co-authored-by: Andrew Werner <awerner32@gmail.com>
Co-authored-by: Renato Costa <renato@cockroachlabs.com>
@craig craig bot closed this as completed in #88670 Sep 26, 2022
ajwerner added a commit to ajwerner/cockroach that referenced this issue Sep 27, 2022
The fundamental race here is that while the slots themselves were being copied
by value, the "any" clauses which are a slice were not. The second bug here is
that the "inline" values were not being properly reset. That bug could lead to
problems when the query was run again in the context of a different element
set. We need to reset those inline values too.

Fixes cockroachdb#88628

Release note: None
@exalate-issue-sync exalate-issue-sync bot added T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) and removed T-sql-schema-deprecated Use T-sql-foundations instead labels May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch-release-22.2 Used to mark GA and release blockers, technical advisories, and bugs for 22.2 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. GA-blocker T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants