sql: connExectuor extraTxnState race condition

[Azhng]

Describe the problem

connExecutor has a field named extraTxnState. This field stores information related to a transaction in addition to the txnState. However, unlike state field, extraTxnState is not protected by a mutex.

This can cause the data race conditions when a transaction state transition happens while crdb_internal.node_sessions virtual table is queried.

Write Operation

Read Operation

Failed CI Run

=== RUN   TestDropColumnAfterMutations
    test_log_scope.go:79: test logs captured to: /go/src/github.com/cockroachdb/cockroach/artifacts/logTestDropColumnAfterMutations3101956907
    test_log_scope.go:80: use -show-logs to present logs inline
=== RUN   TestDropColumnAfterMutations/basic-concurrent-drop-mutations
=== RUN   TestDropColumnAfterMutations/failed-concurrent-drop-mutations
==================
WARNING: DATA RACE
Read at 0x00c00513e3f0 by goroutine 607:
  github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).serialize()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:2892 +0x507
  github.com/cockroachdb/cockroach/pkg/sql.(*SessionRegistry).SerializeAll()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/exec_util.go:1950 +0x244
  github.com/cockroachdb/cockroach/pkg/server.(*baseStatusServer).getLocalSessions()
      /go/src/github.com/cockroachdb/cockroach/pkg/server/status.go:187 +0x34d
  github.com/cockroachdb/cockroach/pkg/server.(*statusServer).ListLocalSessions()
      /go/src/github.com/cockroachdb/cockroach/pkg/server/status.go:2118 +0x84
  github.com/cockroachdb/cockroach/pkg/server/serverpb._Status_ListLocalSessions_Handler.func1()
      /go/src/github.com/cockroachdb/cockroach/pkg/server/serverpb/status.pb.go:6939 +0x8b
  github.com/cockroachdb/cockroach/pkg/util/tracing.ServerInterceptor.func1()
      /go/src/github.com/cockroachdb/cockroach/pkg/util/tracing/grpc_interceptor.go:150 +0x5c2
  google.golang.org/grpc.chainUnaryInterceptors.func1.1()
      /go/src/github.com/cockroachdb/cockroach/vendor/google.golang.org/grpc/server.go:1116 +0x125
  github.com/cockroachdb/cockroach/pkg/rpc.NewServer.func3()
      /go/src/github.com/cockroachdb/cockroach/pkg/rpc/context.go:236 +0xf5
  google.golang.org/grpc.chainUnaryInterceptors.func1.1()
      /go/src/github.com/cockroachdb/cockroach/vendor/google.golang.org/grpc/server.go:1119 +0x226
  github.com/cockroachdb/cockroach/pkg/rpc.kvAuth.unaryInterceptor()
      /go/src/github.com/cockroachdb/cockroach/pkg/rpc/auth.go:71 +0x1c1
  github.com/cockroachdb/cockroach/pkg/rpc.kvAuth.unaryInterceptor-fm()
      /go/src/github.com/cockroachdb/cockroach/pkg/rpc/auth.go:47 +0x99
  google.golang.org/grpc.chainUnaryInterceptors.func1.1()
      /go/src/github.com/cockroachdb/cockroach/vendor/google.golang.org/grpc/server.go:1119 +0x226
  github.com/cockroachdb/cockroach/pkg/rpc.NewServer.func1.1()
      /go/src/github.com/cockroachdb/cockroach/pkg/rpc/context.go:205 +0x70
  github.com/cockroachdb/cockroach/pkg/util/stop.(*Stopper).RunTaskWithErr()
      /go/src/github.com/cockroachdb/cockroach/pkg/util/stop/stopper.go:344 +0x161
  github.com/cockroachdb/cockroach/pkg/rpc.NewServer.func1()
      /go/src/github.com/cockroachdb/cockroach/pkg/rpc/context.go:203 +0x135
  google.golang.org/grpc.chainUnaryInterceptors.func1.1()
      /go/src/github.com/cockroachdb/cockroach/vendor/google.golang.org/grpc/server.go:1119 +0x226
  google.golang.org/grpc.chainUnaryInterceptors.func1()
      /go/src/github.com/cockroachdb/cockroach/vendor/google.golang.org/grpc/server.go:1121 +0x285
  github.com/cockroachdb/cockroach/pkg/server/serverpb._Status_ListLocalSessions_Handler()
      /go/src/github.com/cockroachdb/cockroach/pkg/server/serverpb/status.pb.go:6941 +0x1da
  google.golang.org/grpc.(*Server).processUnaryRPC()
      /go/src/github.com/cockroachdb/cockroach/vendor/google.golang.org/grpc/server.go:1282 +0x1514
  google.golang.org/grpc.(*Server).handleStream()
      /go/src/github.com/cockroachdb/cockroach/vendor/google.golang.org/grpc/server.go:1616 +0xfd3
  google.golang.org/grpc.(*Server).serveStreams.func1.2()
      /go/src/github.com/cockroachdb/cockroach/vendor/google.golang.org/grpc/server.go:921 +0xfd

Previous write at 0x00c00513e3f0 by goroutine 1665:
  github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).txnStateTransitionsApplyWrapper()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:2724 +0x7d0
  github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execCmd()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1955 +0x1ca8
  github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).run()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1703 +0x5fe
  github.com/cockroachdb/cockroach/pkg/sql.(*InternalExecutor).initConnEx.func1()
      /go/src/github.com/cockroachdb/cockroach/pkg/sql/internal.go:205 +0xc4

Expected behavior

Race condition should not happen.

[Azhng]

Looking at the git history, this bug has been present in the code base since at least July, 2020

[ajwerner]

I think synchronization around session state should go to @cockroachdb/sql-experience to triage. At this point that's the team with both the deepest vested interest in making the state of that world better and also with the top-level ownership of the session serialization stuff. I'm happy to be convinced otherwise. It's definitely the case that these integration points around the connExecutor are fragile and mostly unowned.