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: race condition writing to ResolvableFunctionReference.FunctionReference #90965

Closed
ajwerner opened this issue Oct 31, 2022 · 7 comments · Fixed by #90983
Closed

sql: race condition writing to ResolvableFunctionReference.FunctionReference #90965

ajwerner opened this issue Oct 31, 2022 · 7 comments · Fixed by #90983
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@ajwerner
Copy link
Contributor

ajwerner commented Oct 31, 2022 •
edited by cockroach-jira-scripts
Loading

Describe the problem
See

==================
WARNING: DATA RACE
Read at 0x00c008b1dd00 by goroutine 733369:
  github.com/cockroachdb/cockroach/pkg/sql/sem/tree.(*ResolvableFunctionReference).Format()
      <autogenerated>:1 +0x3c
  github.com/cockroachdb/cockroach/pkg/sql/sem/tree.(*FmtCtx).formatNodeOrHideConstants()
      github.com/cockroachdb/cockroach/pkg/sql/sem/tree/hide_constants.go:49 +0x203
  github.com/cockroachdb/cockroach/pkg/sql/sem/tree.(*FmtCtx).FormatNode()
      github.com/cockroachdb/cockroach/pkg/sql/sem/tree/format.go:452 +0x136
  github.com/cockroachdb/cockroach/pkg/sql/sem/tree.(*FuncExpr).Format.func1()
      github.com/cockroachdb/cockroach/pkg/sql/sem/tree/expr.go:1359 +0x47
  github.com/cockroachdb/cockroach/pkg/sql/sem/tree.(*FmtCtx).WithFlags()
      github.com/cockroachdb/cockroach/pkg/sql/sem/tree/format.go:376 +0x130
  github.com/cockroachdb/cockroach/pkg/sql/sem/tree.(*FuncExpr).Format()
      github.com/cockroachdb/cockroach/pkg/sql/sem/tree/expr.go:1358 +0x16c
  github.com/cockroachdb/cockroach/pkg/sql/sem/tree.(*FmtCtx).formatNodeOrHideConstants()
      github.com/cockroachdb/cockroach/pkg/sql/sem/tree/hide_constants.go:49 +0x203
  github.com/cockroachdb/cockroach/pkg/sql/sem/tree.(*FmtCtx).FormatNode()
      github.com/cockroachdb/cockroach/pkg/sql/sem/tree/format.go:452 +0x136
  github.com/cockroachdb/cockroach/pkg/sql/sem/tree.(*SelectExpr).Format()
      github.com/cockroachdb/cockroach/pkg/sql/sem/tree/select.go:184 +0x66
  github.com/cockroachdb/cockroach/pkg/sql/sem/tree.(*FmtCtx).formatNodeOrHideConstants()
      github.com/cockroachdb/cockroach/pkg/sql/sem/tree/hide_constants.go:49 +0x203
  github.com/cockroachdb/cockroach/pkg/sql/sem/tree.(*FmtCtx).FormatNode()
      github.com/cockroachdb/cockroach/pkg/sql/sem/tree/format.go:452 +0x136
  github.com/cockroachdb/cockroach/pkg/sql/sem/tree.(*SelectExprs).Format()
      github.com/cockroachdb/cockroach/pkg/sql/sem/tree/select.go:151 +0x64
  github.com/cockroachdb/cockroach/pkg/sql/sem/tree.(*FmtCtx).formatNodeOrHideConstants()
      github.com/cockroachdb/cockroach/pkg/sql/sem/tree/hide_constants.go:49 +0x203
  github.com/cockroachdb/cockroach/pkg/sql/sem/tree.(*FmtCtx).FormatNode()
      github.com/cockroachdb/cockroach/pkg/sql/sem/tree/format.go:452 +0x136
  github.com/cockroachdb/cockroach/pkg/sql/sem/tree.(*SelectClause).Format()
      github.com/cockroachdb/cockroach/pkg/sql/sem/tree/select.go:118 +0x1f0
  github.com/cockroachdb/cockroach/pkg/sql/sem/tree.(*FmtCtx).formatNodeOrHideConstants()
      github.com/cockroachdb/cockroach/pkg/sql/sem/tree/hide_constants.go:49 +0x203
  github.com/cockroachdb/cockroach/pkg/sql/sem/tree.(*FmtCtx).FormatNode()
      github.com/cockroachdb/cockroach/pkg/sql/sem/tree/format.go:452 +0x136
  github.com/cockroachdb/cockroach/pkg/sql/sem/tree.(*Select).Format()
      github.com/cockroachdb/cockroach/pkg/sql/sem/tree/select.go:57 +0x96
  github.com/cockroachdb/cockroach/pkg/sql/sem/tree.(*FmtCtx).formatNodeOrHideConstants()
      github.com/cockroachdb/cockroach/pkg/sql/sem/tree/hide_constants.go:49 +0x203
  github.com/cockroachdb/cockroach/pkg/sql/sem/tree.(*FmtCtx).FormatNode()
      github.com/cockroachdb/cockroach/pkg/sql/sem/tree/format.go:452 +0x136
  github.com/cockroachdb/cockroach/pkg/sql/sem/tree.AsStringWithFlags()
      github.com/cockroachdb/cockroach/pkg/sql/sem/tree/format.go:590 +0x8d
  github.com/cockroachdb/cockroach/pkg/sql.formatStatementHideConstants()
      github.com/cockroachdb/cockroach/pkg/sql/exec_util.go:3427 +0xc57
  github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).serialize()
      github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:3180 +0xbf8
  github.com/cockroachdb/cockroach/pkg/sql.(*SessionRegistry).SerializeAllLocked()
      github.com/cockroachdb/cockroach/pkg/sql/exec_util.go:2118 +0x1a4
  github.com/cockroachdb/cockroach/pkg/server.(*baseStatusServer).getLocalSessions()
      github.com/cockroachdb/cockroach/pkg/server/status.go:219 +0x3c8
  github.com/cockroachdb/cockroach/pkg/server.(*statusServer).ListLocalSessions()
      github.com/cockroachdb/cockroach/pkg/server/status.go:2623 +0x84
  github.com/cockroachdb/cockroach/pkg/server/serverpb._Status_ListLocalSessions_Handler.func1()
      github.com/cockroachdb/cockroach/pkg/server/serverpb/bazel-out/k8-fastbuild/bin/pkg/server/serverpb/serverpb_go_proto_/github.com/cockroachdb/cockroach/pkg/server/serverpb/status.pb.go:8366 +0x8b
  github.com/cockroachdb/cockroach/pkg/util/tracing/grpcinterceptor.ServerInterceptor.func1()
      github.com/cockroachdb/cockroach/pkg/util/tracing/grpcinterceptor/grpc_interceptor.go:114 +0x634
  google.golang.org/grpc.chainUnaryInterceptors.func1.1()
      google.golang.org/grpc/external/org_golang_google_grpc/server.go:1117 +0x125
  github.com/cockroachdb/cockroach/pkg/rpc.NewServerEx.func3()
      github.com/cockroachdb/cockroach/pkg/rpc/pkg/rpc/context.go:263 +0xf8
  google.golang.org/grpc.chainUnaryInterceptors.func1.1()
      google.golang.org/grpc/external/org_golang_google_grpc/server.go:1120 +0x22a
  github.com/cockroachdb/cockroach/pkg/rpc.kvAuth.unaryInterceptor()
      github.com/cockroachdb/cockroach/pkg/rpc/pkg/rpc/auth.go:73 +0x1c1
  github.com/cockroachdb/cockroach/pkg/rpc.kvAuth.unaryInterceptor-fm()
      <autogenerated>:1 +0x99
  google.golang.org/grpc.chainUnaryInterceptors.func1.1()
      google.golang.org/grpc/external/org_golang_google_grpc/server.go:1120 +0x22a
  github.com/cockroachdb/cockroach/pkg/rpc.NewServerEx.func1.1()
      github.com/cockroachdb/cockroach/pkg/rpc/pkg/rpc/context.go:232 +0x70
  github.com/cockroachdb/cockroach/pkg/util/stop.(*Stopper).RunTaskWithErr()
      github.com/cockroachdb/cockroach/pkg/util/stop/stopper.go:341 +0x147
  github.com/cockroachdb/cockroach/pkg/rpc.NewServerEx.func1()
      github.com/cockroachdb/cockroach/pkg/rpc/pkg/rpc/context.go:230 +0x137
  google.golang.org/grpc.chainUnaryInterceptors.func1.1()
      google.golang.org/grpc/external/org_golang_google_grpc/server.go:1120 +0x22a
  google.golang.org/grpc.chainUnaryInterceptors.func1()
      google.golang.org/grpc/external/org_golang_google_grpc/server.go:1122 +0x298
  github.com/cockroachdb/cockroach/pkg/server/serverpb._Status_ListLocalSessions_Handler()
      github.com/cockroachdb/cockroach/pkg/server/serverpb/bazel-out/k8-fastbuild/bin/pkg/server/serverpb/serverpb_go_proto_/github.com/cockroachdb/cockroach/pkg/server/serverpb/status.pb.go:8368 +0x1dd
  google.golang.org/grpc.(*Server).processUnaryRPC()
      google.golang.org/grpc/external/org_golang_google_grpc/server.go:1283 +0x1409
  google.golang.org/grpc.(*Server).handleStream()
      google.golang.org/grpc/external/org_golang_google_grpc/server.go:1620 +0xff8
  google.golang.org/grpc.(*Server).serveStreams.func1.2()
      google.golang.org/grpc/external/org_golang_google_grpc/server.go:922 +0xec
Previous write at 0x00c008b1dd00 by goroutine 733356:
  github.com/cockroachdb/cockroach/pkg/sql/sem/tree.(*ResolvableFunctionReference).Resolve()
      github.com/cockroachdb/cockroach/pkg/sql/sem/tree/function_name.go:114 +0x344
  github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder.(*scope).VisitPre()
      github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder/scope.go:1061 +0x224
  github.com/cockroachdb/cockroach/pkg/sql/sem/tree.WalkExpr()
      github.com/cockroachdb/cockroach/pkg/sql/sem/tree/walk.go:825 +0x7a
  github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder.(*scope).walkExprTree()
      github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder/scope.go:428 +0x8d
  github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder.(*scope).resolveType()
      github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder/scope.go:467 +0x4f
  github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder.(*Builder).analyzeSelectList()
      github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder/project.go:160 +0x37b
  github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder.(*Builder).analyzeProjectionList()
      github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder/project.go:94 +0x3b9
  github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder.(*Builder).buildSelectClause()
      github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder/select.go:1065 +0x41c
  github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder.(*Builder).buildSelectStmtWithoutParens()
      github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder/select.go:1002 +0x25c
  github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder.(*Builder).buildSelect.func1()
      github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder/select.go:971 +0x14c
  github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder.(*Builder).processWiths()
      github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder/with.go:116 +0xcf
  github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder.(*Builder).buildSelect()
      github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder/select.go:970 +0x727
  github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder.(*Builder).buildStmt()
      github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder/builder.go:305 +0xb0e
  github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder.(*Builder).buildStmtAtRoot()
      github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder/builder.go:252 +0x2ef
  github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder.(*Builder).Build()
      github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder/builder.go:226 +0x5e7
  github.com/cockroachdb/cockroach/pkg/sql.(*optPlanningCtx).buildExecMemo()
      github.com/cockroachdb/cockroach/pkg/sql/plan_opt.go:563 +0xaf6
  github.com/cockroachdb/cockroach/pkg/sql.(*planner).makeOptimizerPlan()
      github.com/cockroachdb/cockroach/pkg/sql/plan_opt.go:231 +0x1c9
  github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).makeExecPlan()
      github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:1471 +0x72
  github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).dispatchToExecutionEngine()
      github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:1096 +0x434
  github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execStmtInOpenState()
      github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:725 +0x37a9
  github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execStmt.func1()
      github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:129 +0x18e
  github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execWithProfiling()
      github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:2421 +0x50f
  github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execStmt()
      github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:128 +0x6cd
  github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execCmd.func1()
      github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1922 +0x790
  github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execCmd()
      github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1927 +0x1cba
  github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).run()
      github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1847 +0x424
  github.com/cockroachdb/cockroach/pkg/sql.(*Server).ServeConn()
      github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:828 +0x1be
  github.com/cockroachdb/cockroach/pkg/sql/pgwire.(*conn).processCommandsAsync.func1()
      github.com/cockroachdb/cockroach/pkg/sql/pgwire/conn.go:728 +0x54a
Goroutine 733369 (running) created at:
  google.golang.org/grpc.(*Server).serveStreams.func1()
      google.golang.org/grpc/external/org_golang_google_grpc/server.go:920 +0x4dd
  google.golang.org/grpc/internal/transport.(*http2Server).operateHeaders()
      google.golang.org/grpc/internal/transport/external/org_golang_google_grpc/internal/transport/http2_server.go:600 +0x3209
  google.golang.org/grpc/internal/transport.(*http2Server).HandleStreams()
      google.golang.org/grpc/internal/transport/external/org_golang_google_grpc/internal/transport/http2_server.go:645 +0x378
  google.golang.org/grpc.(*Server).serveStreams()
      google.golang.org/grpc/external/org_golang_google_grpc/server.go:906 +0x275
  google.golang.org/grpc.(*Server).handleRawConn.func1()
      google.golang.org/grpc/external/org_golang_google_grpc/server.go:848 +0x64
Goroutine 733356 (running) created at:
  github.com/cockroachdb/cockroach/pkg/sql/pgwire.(*conn).processCommandsAsync()
      github.com/cockroachdb/cockroach/pkg/sql/pgwire/conn.go:639 +0x3a4
  github.com/cockroachdb/cockroach/pkg/sql/pgwire.(*conn).serveImpl()
      github.com/cockroachdb/cockroach/pkg/sql/pgwire/conn.go:360 +0xcd1
  github.com/cockroachdb/cockroach/pkg/sql/pgwire.(*Server).serveConn()
      github.com/cockroachdb/cockroach/pkg/sql/pgwire/conn.go:174 +0x484
  github.com/cockroachdb/cockroach/pkg/sql/pgwire.(*Server).ServeConn()
      github.com/cockroachdb/cockroach/pkg/sql/pgwire/server.go:878 +0x108b
  github.com/cockroachdb/cockroach/pkg/server.(*SQLServer).startServeSQL.func1.1()
      github.com/cockroachdb/cockroach/pkg/server/server_sql.go:1517 +0x124
  github.com/cockroachdb/cockroach/pkg/util/netutil.(*Server).ServeWith.func1()
      github.com/cockroachdb/cockroach/pkg/util/netutil/net.go:162 +0x178
  github.com/cockroachdb/cockroach/pkg/util/stop.(*Stopper).RunAsyncTaskEx.func2()
      github.com/cockroachdb/cockroach/pkg/util/stop/stopper.go:489 +0x1f6
==================

in https://teamcity.cockroachdb.com/buildConfiguration/Cockroach_BazelExtendedCi/7224947?showRootCauses=false&expandBuildChangesSection=true&expandBuildProblemsSection=true&expandBuildTestsSection=true

Jira issue: CRDB-21045
@ajwerner ajwerner added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Oct 31, 2022
@blathers-crl blathers-crl bot added T-sql-queries SQL Queries Team T-sql-schema-deprecated Use T-sql-foundations instead labels Oct 31, 2022
@ajwerner
Copy link
Contributor Author

@chengxiong-ruan and @mgartner, this one is interesting. It reveals to me that we don't have a clear understanding of when it's safe to modify the AST. @jordanlewis maybe this is fallout from #86968 which assumed that it could safely look at an AST which is in fact, sadly, volatile?

@ajwerner ajwerner mentioned this issue Oct 31, 2022
Merged
@jordanlewis
Copy link
Member

Argh, yes, that's exactly right. The AST should be completely immutable, but there are still a few pesky spots where we edit it - maybe ResolvableFunctionReference is the last one, though?

See old places where we (I) have tripped on this:

#28033 (comment)
#46046

@jordanlewis
Copy link
Member

We might need to revert that PR, I'm not sure there's an easy fix

@jordanlewis
Copy link
Member

And see this issue: #22847 :(

@ajwerner
Copy link
Contributor Author

https://teamcity.cockroachdb.com/buildConfiguration/Cockroach_BazelExtendedCi/7235967?showRootCauses=false&expandBuildChangesSection=true&expandBuildProblemsSection=true&expandBuildTestsSection=true

@jordanlewis
Copy link
Member

Will deal with this by EOD one way or another.

@jordanlewis jordanlewis mentioned this issue Oct 31, 2022
Merged
@chengxiong-ruan
Copy link
Contributor

Yeah, the question of at which point a function reference is a resolved/typechecked one was a pain when I refactored the function resolution with UDF, which made us had to use some hack to determine function properties :( It definitely worth the effort to clean that up, but this is a hard one as it tangles with many places of the execution path (which is still a land of magic to me after touching several places of it :( ).

@craig craig bot closed this as completed in e2de080 Oct 31, 2022
@exalate-issue-sync exalate-issue-sync bot removed the T-sql-queries SQL Queries Team label Oct 31, 2022
@michae2 michae2 mentioned this issue Oct 31, 2022
Merged
@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
@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
No one assigned
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
SQL Queries
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

sql: fix data race in session serialization jordanlewis/cockroach
3 participants
@jordanlewis @ajwerner @chengxiong-ruan