Merge #27262 #27307 #27329 #27348

27262: ui: require yarn >= 1.7.0 r=benesch,vilterp a=tschottdorf

Builds before the upgrade failed mysteriously.
@benesch (cc) suspects a bug in older versions of yarn.

```
$ make build
Running make with -j8
GOPATH set to /Users/tschottdorf/go
build/node-run.sh -C pkg/ui yarn install --offline
(echo "// Code generated by goyacc. DO NOT EDIT."; \
     echo "// GENERATED FILE DO NOT EDIT"; \
     cat pkg/sql/parser/gen/sql.go.tmp | \
     sed -E 's/^const ([A-Z][_A-Z0-9]*) =.*$/const \1 = lex.\1/g') > pkg/sql/parser/sql.go.tmp || rm pkg/sql/parser/sql.go.tmp
(echo "// Code generated by make. DO NOT EDIT."; \
     echo "// GENERATED FILE DO NOT EDIT"; \
     echo; \
     echo "package lex"; \
     echo; \
     grep '^const [A-Z][_A-Z0-9]* ' pkg/sql/parser/gen/sql.go.tmp) > pkg/sql/lex/tokens.go.tmp || rm pkg/sql/lex/tokens.go.tmp
mv -f pkg/sql/lex/tokens.go.tmp pkg/sql/lex/tokens.go
mv -f pkg/sql/parser/sql.go.tmp pkg/sql/parser/sql.go
yarn install v1.6.0
(node:50064) [DEP0005] DeprecationWarning: Buffer() is deprecated due to security and usability issues. Please use the Buffer.alloc(), Buffer.allocUnsafe(), or Buffer.from() methods instead.
[1/5] Validating package.json...
go install -v docgen
bin/prereqs ./pkg/cmd/docgen > bin/docgen.d.tmp
[2/5] Resolving packages...
mv -f bin/docgen.d.tmp bin/docgen.d
[3/5] Fetching packages...
build/node-run.sh pkg/ui/bin/gen-protobuf-cli-deps.js > pkg/ui/node_modules/protobufjs/cli/package.json
bash: pkg/ui/node_modules/protobufjs/cli/package.json: No such file or directory
make: *** [pkg/ui/yarn.installed] Error 1
make: *** Waiting for unfinished jobs....
```

Release note (general change): the build now requires yarn at version
1.7.0 or above, to work around yarnpkg/yarn#5761.

27307: storage: return TransactionRetryError from QueryIntent on pushed intent r=nvanbenschoten a=nvanbenschoten

Extracted from #26599.

This change adjusts how QueryIntent handles pushed intents. First, it
changes how pushed intents interact with SNAPSHOT transactions. Next,
it makes sure to update the response transaction in the case of pushed
intents. Finally, it returns a RETRY_SERIALIZABLE TransactionRetryError
for SERIALIZABLE transactions who observe a pushed intent with the
RETURN_ERROR behavior.

Release note: None

27329: log: use fake stacktrace when test-exiting r=benesch a=tschottdorf

Having stack traces littered across tests has been a long-standing pet
peeve of mine (and presumably others). We search through logs more often
than we would like to admit, and having to skip a number of intentional
stack traces is annoying.

Make it so that whenever the logging exit func is mocked, the stack
trace indicates that.

Release note: None

27348: engine: fix RocksDBBatchReader example comment r=nvanbenschoten a=nvanbenschoten

Release note: None

Co-authored-by: Tobias Schottdorf <[email protected]>
Co-authored-by: Nathan VanBenschoten <[email protected]>

CONTRIBUTING.md

@@ -43,7 +43,7 @@ new engineers.
      - GNU Make (3.81+ is known to work)
      - CMake 3.1+
      - Autoconf 2.68+
-     - NodeJS 6.x and Yarn 1.0+
+     - NodeJS 6.x and Yarn 1.7+
 
    Note that at least 4GB of RAM is required to build from source and run tests.
 

pkg/cli/backtrace.go

@@ -93,7 +93,7 @@ func initBacktrace(logDir string, options ...stop.Option) *stop.Stopper {
 	bcd.Register(tracer)
 
 	// Hook log.Fatal*.
-	log.SetExitFunc(func(code int) {
+	log.SetExitFunc(false /* hideStack */, func(code int) {
 		_ = bcd.Trace(tracer, fmt.Errorf("exit %d", code), nil)
 		os.Exit(code)
 	})

pkg/cmd/allocsim/main.go

@@ -522,7 +522,7 @@ func main() {
 	a := newAllocSim(c)
 	a.localities = localities
 
-	log.SetExitFunc(func(code int) {
+	log.SetExitFunc(false /* hideStack */, func(code int) {
 		c.Close()
 		os.Exit(code)
 	})

pkg/cmd/zerosum/main.go

@@ -441,7 +441,7 @@ func main() {
 	c := &localcluster.LocalCluster{Cluster: localcluster.New(cfg)}
 	defer c.Close()
 
-	log.SetExitFunc(func(code int) {
+	log.SetExitFunc(false /* hideStack */, func(code int) {
 		c.Close()
 		os.Exit(code)
 	})

pkg/roachpb/api.proto

@@ -749,9 +749,12 @@ message QueryIntentRequest {
   // be committed by the provided transaction. If an intent is found at the
   // specified key, the intent is only considered a match if it has the same ID,
   // the same epoch, and a provisional commit timestamp that is equal to or less
-  // than that in the provided transaction. If the intent's timestamp is greater
-  // than that in the provided transaction, it would prevent the transaction
-  // from committing and is therefore not a match.
+  // than that in the provided transaction. For SERIALIZABLE transactions, if
+  // the intent's timestamp is greater than that in the provided transaction, it
+  // would prevent the transaction from committing and is therefore not a match.
+  // However, for SNAPSHOT transactions, if the intent's timestamp is greater
+  // than that in the provided transaction, it would not prevent the transaction
+  // from committing and therefore is a match.
   //
   // Additionally, the intent is only considered a match if its sequence number
   // is equal to or greater than the expected txn's sequence number. The
@@ -767,7 +770,9 @@ message QueryIntentRequest {
     // Don't do anything special, just note that the intent was not found in the
     // response.
     DO_NOTHING = 0;
-    // Return an IntentMissingError.
+    // Return an IntentMissingError. Special-cased to return a SERIALIZABLE
+    // retry error if a SERIALIZABLE transaction queries its own intent and
+    // finds it has been pushed.
     RETURN_ERROR = 1;
     // Prevent the intent from ever being written in the future. If set as the
     // behavior, a response with found_intent=false implies that an intent will

pkg/server/server_test.go

@@ -815,8 +815,8 @@ func TestPersistHLCUpperBound(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 
 	var fatal bool
-	defer log.SetExitFunc(os.Exit)
-	log.SetExitFunc(func(r int) {
+	defer log.ResetExitFunc()
+	log.SetExitFunc(true /* hideStack */, func(r int) {
 		defer log.Flush()
 		if r != 0 {
 			fatal = true

pkg/server/version_cluster_test.go

@@ -17,7 +17,6 @@ package server_test
 import (
 	"context"
 	gosql "database/sql"
-	"os"
 	"path/filepath"
 	"strconv"
 	"sync/atomic"
@@ -320,8 +319,8 @@ func TestClusterVersionMixedVersionTooOld(t *testing.T) {
 
 	exits := make(chan int, 100)
 
-	log.SetExitFunc(func(i int) { exits <- i })
-	defer log.SetExitFunc(os.Exit)
+	log.SetExitFunc(true /* hideStack */, func(i int) { exits <- i })
+	defer log.ResetExitFunc()
 
 	// Three nodes at v1.1 and a fourth one at 1.0, but all operating at v1.0.
 	versions := [][2]string{{"1.0", "1.1"}, {"1.0", "1.1"}, {"1.0", "1.1"}, {"1.0", "1.0"}}
@@ -376,8 +375,8 @@ func TestClusterVersionMixedVersionTooNew(t *testing.T) {
 
 	exits := make(chan int, 100)
 
-	log.SetExitFunc(func(i int) { exits <- i })
-	defer log.SetExitFunc(os.Exit)
+	log.SetExitFunc(true /* hideStack */, func(i int) { exits <- i })
+	defer log.ResetExitFunc()
 
 	// Three nodes at v1.1 and a fourth one (started later) at 1.1-2 (and
 	// incompatible with anything earlier).

pkg/sqlmigrations/migrations_test.go

@@ -18,7 +18,6 @@ import (
 	"bytes"
 	"context"
 	"fmt"
-	"os"
 	"strings"
 	"testing"
 	"time"
@@ -335,8 +334,8 @@ func TestLeaseExpiration(t *testing.T) {
 	defer func() { leaseRefreshInterval = oldLeaseRefreshInterval }()
 
 	exitCalled := make(chan bool)
-	log.SetExitFunc(func(int) { exitCalled <- true })
-	defer log.SetExitFunc(os.Exit)
+	log.SetExitFunc(true /* hideStack */, func(int) { exitCalled <- true })
+	defer log.ResetExitFunc()
 	// Disable stack traces to make the test output in teamcity less deceiving.
 	defer log.DisableTracebacks()()
 

pkg/storage/batcheval/cmd_query_intent.go

@@ -38,6 +38,7 @@ func QueryIntent(
 	ctx context.Context, batch engine.ReadWriter, cArgs CommandArgs, resp roachpb.Response,
 ) (result.Result, error) {
 	args := cArgs.Args.(*roachpb.QueryIntentRequest)
+	h := cArgs.Header
 	reply := resp.(*roachpb.QueryIntentResponse)
 
 	// Snapshot transactions cannot be prevented using a QueryIntent command.
@@ -65,7 +66,11 @@ func QueryIntent(
 		return result.Result{}, err
 	}
 
+	// Determine if the request is querying an intent in its own transaction.
+	ownTxn := h.Txn != nil && h.Txn.ID == args.Txn.ID
+
 	var curIntent *roachpb.Intent
+	var curIntentPushed bool
 	switch len(intents) {
 	case 0:
 		reply.FoundIntent = false
@@ -75,8 +80,27 @@ func QueryIntent(
 		// comparison.
 		reply.FoundIntent = (args.Txn.ID == curIntent.Txn.ID) &&
 			(args.Txn.Epoch == curIntent.Txn.Epoch) &&
-			(!args.Txn.Timestamp.Less(curIntent.Txn.Timestamp)) &&
 			(args.Txn.Sequence <= curIntent.Txn.Sequence)
+
+		// Check whether the intent was pushed past its expected timestamp.
+		if reply.FoundIntent && args.Txn.Timestamp.Less(curIntent.Txn.Timestamp) {
+			// The intent matched but was pushed to a later timestamp.
+			curIntentPushed = true
+
+			// If the transaction is SERIALIZABLE, consider a pushed intent as a
+			// missing intent. If the transaction is SNAPSHOT, don't.
+			if args.Txn.Isolation == enginepb.SERIALIZABLE {
+				reply.FoundIntent = false
+			}
+
+			// If the request was querying an intent in its own transaction, update
+			// the response transaction.
+			if ownTxn {
+				clonedTxn := h.Txn.Clone()
+				reply.Txn = &clonedTxn
+				reply.Txn.Timestamp.Forward(curIntent.Txn.Timestamp)
+			}
+		}
 	default:
 		log.Fatalf(ctx, "more than 1 intent on single key: %v", intents)
 	}
@@ -86,6 +110,13 @@ func QueryIntent(
 		case roachpb.QueryIntentRequest_DO_NOTHING:
 			// Do nothing.
 		case roachpb.QueryIntentRequest_RETURN_ERROR:
+			if ownTxn && curIntentPushed {
+				// If the transaction's own intent was pushed, go ahead and
+				// return a TransactionRetryError immediately with an updated
+				// transaction proto. This is an optimization that can help
+				// the txn use refresh spans more effectively.
+				return result.Result{}, roachpb.NewTransactionRetryError(roachpb.RETRY_SERIALIZABLE)
+			}
 			return result.Result{}, roachpb.NewIntentMissingError(curIntent)
 		case roachpb.QueryIntentRequest_PREVENT:
 			// The intent will be prevented by bumping the timestamp cache for

pkg/storage/engine/batch.go

@@ -407,7 +407,7 @@ func rocksDBBatchVarString(repr []byte) (s []byte, orepr []byte, err error) {
 // 	   fmt.Printf("merge(%x,%x)", r.Key(), r.Value())
 // 	 }
 // }
-// if err != nil {
+// if err := r.Error(); err != nil {
 //   return err
 // }
 type RocksDBBatchReader struct {