Can you help investigate? The acceptance tests are failing with
localcluster.go:679: unexpected extra event &{0 die} (after [])

Never saw this before.

I haven't looked at anything, but I believe that acceptance error means that a node has crashed. We have some magic listening to docker container events 8-)

Great, thanks! LGTM modulo one typo.

Reviewed 1 of 2 files at r1, 5 of 5 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

pkg/server/server.go, line 471 at r2 (raw file):

  s.stopper.RunWorker(func() {
      netutil.FatalIfUnexpected(httpServer.ServeWith(s.stopper, pgL, func(conn net.Conn) {
          pgCtx := s.pgServer.AmbientCtx.AnnotateCtx(context.Background())

can init this once, before running netutil.FatalIf..

pkg/server/server.go, line 472 at r2 (raw file):

      netutil.FatalIfUnexpected(httpServer.ServeWith(s.stopper, pgL, func(conn net.Conn) {
          pgCtx := s.pgServer.AmbientCtx.AnnotateCtx(context.Background())
          pgCtx = log.WithLogTagStr(ctx, "client", conn.RemoteAddr().String())

s/ctx/pgCtx. Maybe just use ctx instead of pgCtx (assuming the linter doesn't complain).

pkg/sql/session.go, line 379 at r1 (raw file):

// This needs to be called before resetForNewSQLTransaction() is called for
// starting another SQL txn.
func (ts *txnState) finishSQLTxn(ctx context.Context) {

maybe name sessionCtx since we are also using a different context ts.Ctx in there

pkg/sql/pgwire/v3.go, line 653 at r2 (raw file):

) error {
  tracing.AnnotateTrace()
  results := c.executor.ExecuteStatements(c.session, stmts, pinfo)

We should probably pass ctx into the executor and make use of it there as well, can you add a TODO?

Comments from Reviewable

Review status: 1 of 6 files reviewed at latest revision, 4 unresolved discussions.

pkg/server/server.go, line 471 at r2 (raw file):

pkg/server/server.go, line 472 at r2 (raw file):

pkg/sql/session.go, line 379 at r1 (raw file):

pkg/sql/pgwire/v3.go, line 653 at r2 (raw file):

(Incidentally this made me think of filing #10096. Not directly relevant but still somewhat.)

Comments from Reviewable

Review status: 1 of 6 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

pkg/server/server.go, line 472 at r3 (raw file):

      pgCtx := s.pgServer.AmbientCtx.AnnotateCtx(context.Background())
      netutil.FatalIfUnexpected(httpServer.ServeWith(s.stopper, pgL, func(conn net.Conn) {
          connCtx = log.WithLogTagStr(pgCtx, "client", conn.RemoteAddr().String())

shouldn't this be :=? (same below)

pkg/sql/pgwire/v3.go, line 653 at r2 (raw file):

Comments from Reviewable

TFYR!

I've not been able to test because I can't get cockroach to build:

# github.com/cockroachdb/cockroach/pkg/storage/engine/rocksdb
pkg/storage/engine/rocksdb/db.cc: In function 'DBStatus DBEngineAddFile(DBEngine*, DBSlice)':
pkg/storage/engine/rocksdb/db.cc:2028:59: warning: 'virtual rocksdb::Status rocksdb::DB::AddFile(const string&, bool)' is deprecated (declared at ../../../../../c-rocksdb/internal/include/rocksdb/db.h:840) [-Wdeprecated-declarations]
   rocksdb::Status status = db->rep->AddFile(ToString(path));
                                                           ^
pkg/storage/engine/rocksdb/db.cc: In constructor 'DBSstFileWriter::DBSstFileWriter(rocksdb::Options*)':
pkg/storage/engine/rocksdb/db.cc:2043:59: error: no matching function for call to 'rocksdb::SstFileWriter::SstFileWriter(rocksdb::EnvOptions, rocksdb::ImmutableCFOptions&, const rocksdb::Comparator*&)'
         rep(rocksdb::EnvOptions(), ioptions, o->comparator) {
                                                           ^
pkg/storage/engine/rocksdb/db.cc:2043:59: note: candidates are:
In file included from ../../../../../c-rocksdb/internal/include/rocksdb/db.h:24:0,
                 from pkg/storage/engine/rocksdb/db.cc:25:
../../../../../c-rocksdb/internal/include/rocksdb/sst_file_writer.h:52:3: note: rocksdb::SstFileWriter::SstFileWriter(const rocksdb::EnvOptions&, const rocksdb::Options&, const rocksdb::Comparator*)
   SstFileWriter(const EnvOptions& env_options, const Options& options,
   ^
../../../../../c-rocksdb/internal/include/rocksdb/sst_file_writer.h:52:3: note:   no known conversion for argument 2 from 'rocksdb::ImmutableCFOptions' to 'const rocksdb::Options&'
../../../../../c-rocksdb/internal/include/rocksdb/sst_file_writer.h:50:7: note: constexpr rocksdb::SstFileWriter::SstFileWriter(const rocksdb::SstFileWriter&)
 class SstFileWriter {
       ^
../../../../../c-rocksdb/internal/include/rocksdb/sst_file_writer.h:50:7: note:   candidate expects 1 argument, 3 provided
Makefile:79: recipe for target 'install' failed
make: *** [install] Error 2

Are there nightly build available anywhere?

Which platform are you building for?

Ubuntu 14.04 x86_64. Used to docker container to run the build

@alamaison RocksDB 4.9 changed some APIs we're using. We currently use RocksDB 4.8. This is all supposed to work automagically via glock and the GLOCKFILE. Make sure that the SHA you have checked out in github.com/cockroachdb/c-rocksdb matches the SHA for that repo in GLOCKFILE.

glock sync github.com/cockroachdb/cockroach

Thanks. sync fixed it. Was on RocksDB 4.11 without it.

P.S. I meant to comment on #10087 but accidentally posted here instead

pkg/server/server.go, line 470 at r4 (raw file):

  s.stopper.RunWorker(func() {
      pgCtx := s.pgServer.AmbientCtx.AnnotateCtx(context.Background())

If this is the only place we are using pgServer.AmbientCtx, why embed it there at all? We might as well j ust do s.AnnotateCtx here no?

Comments from Reviewable

@RaduBerinde perhaps, but I was following the comments next to the definition of AmbientContext, which was instructing me to "plumb AmbientContext to all the servers". Perhaps we could talk about why we would want multiple ambient contexts as opposed to a single one?

We want multiple contexts because each layer's ambient context can add tags (and other stuff) to the parent's (server has nX, store has nX,sY, replica has nX,sY,rZ).

The intention is to be embedded in servers/components that use it, in our case it's the Server code that annotates the context, so it seems a little weird that we copy and use it from the pgServer. I think that maybe the code for these workers belongs in pgServer not in Server, and that's the culprit of the weirdness. Either way, it's not a big deal, just wanted to see if I'm missing something.