Merge #69093 #69104 #69111 #69112

69093: kvserver: remove unused args from r.evaluateCommand r=andreimatei a=andreimatei

Release note: None

69104: colexecwindow: remove unnecessary copy from relative rank operators r=DrewKimball a=DrewKimball

Previously, relative rank window function operators performed an
unnecessary copy on input batches - this was an artifact from when
`SpillingQueue` retained references to enqueued batches. This patch
removes the copy.

Release note: None

69111: rfcs: update ON UPDATE RFC to note syntax change r=mgartner,otan a=pawalt

After starting ON UPDATE implementation, we realized we had a problem
with syntax ambiguity. We've moved to slightly different syntax as a
result. This PR updates the RFC to match the current state of the
implementation.

Release note: None

69112: bazel: add bazel testrace CI job r=rail a=rickystewart

Also some light refactoring: add a `--config race` and use that instead
of directly referencing the `--@io_bazel_rules_go//go/config:race` flag.
Also add `GORACE=halt_on_error=1` to the test environment when running
under `--config race` -- this makes the behavior match up more closely
to the `Makefile`.

Closes #67145.

Release note: None

Co-authored-by: Andrei Matei <[email protected]>
Co-authored-by: Drew Kimball <[email protected]>
Co-authored-by: Peyton Walters <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>

.bazelrc

@@ -1,6 +1,7 @@
 build --symlink_prefix=_bazel/ --ui_event_filters=-DEBUG --define gotags=bazel,crdb_test_off,gss --experimental_proto_descriptor_sets_include_source_info
 test --config=test
 build:test --define gotags=bazel,crdb_test,gss --test_env=TZ=
+build:race --@io_bazel_rules_go//go/config:race --test_env=GORACE=halt_on_error=1
 query --ui_event_filters=-DEBUG
 
 try-import %workspace%/.bazelrc.user

build/teamcity/cockroach/ci/tests/testrace.sh

@@ -0,0 +1,38 @@
+#!/usr/bin/env bash
+
+set -euo pipefail
+
+dir="$(dirname $(dirname $(dirname $(dirname $(dirname "${0}")))))"
+
+source "$dir/teamcity-support.sh"  # For $root
+source "$dir/teamcity-bazel-support.sh"  # For run_bazel
+
+tc_prepare
+
+tc_start_block "Determine changed packages"
+if tc_release_branch; then
+  pkgspec=./pkg/...
+  echo "On release branch ($TC_BUILD_BRANCH), so running testrace on all packages ($pkgspec)"
+else
+  pkgspec=$(changed_go_pkgs)
+  if [[ $(echo "$pkgspec" | wc -w) -gt 10 ]]; then
+    echo "PR #$TC_BUILD_BRANCH changed many packages; skipping race detector tests"
+    exit 0
+  elif [[ -z "$pkgspec" ]]; then
+    echo "PR #$TC_BUILD_BRANCH has no changed packages; skipping race detector tests"
+    exit 0
+  fi
+  if [[ $pkgspec == *"./pkg/sql/opt"* ]]; then
+    # If one opt package was changed, run all opt packages (the optimizer puts
+    # various checks behind the race flag to keep them out of release builds).
+    echo "$pkgspec" | sed 's$./pkg/sql/opt/[^ ]*$$g'
+    pkgspec=$(echo "$pkgspec" | sed 's$./pkg/sql/opt[^ ]*$$g')
+    pkgspec="$pkgspec ./pkg/sql/opt/..."
+  fi
+  echo "PR #$TC_BUILD_BRANCH has changed packages; running race detector tests on $pkgspec"
+fi
+tc_end_block "Determine changed packages"
+
+tc_start_block "Run race detector tests"
+run_bazel build/teamcity/cockroach/ci/tests/testrace_impl.sh $pkgspec
+tc_end_block "Run race detector tests"

build/teamcity/cockroach/ci/tests/testrace_impl.sh

@@ -0,0 +1,27 @@
+#!/usr/bin/env bash
+
+set -xeuo pipefail
+
+# Usage: testrace_impl.sh PKG1 [PKG2 PKG3 PKG4...]
+# packages are expected to be formatted as go-style, e.g. ./pkg/cmd/bazci.
+
+bazel build //pkg/cmd/bazci --config=ci
+for pkg in "$@"
+do
+    # Query to list all affected tests.
+    pkg=${pkg#"./"}
+    if [[ $(basename $pkg) != ... ]]
+    then
+        pkg="$pkg:all"
+    fi
+    tests=$(bazel query "kind(go_test, $pkg)" --output=label)
+
+    # Run affected tests.
+    for test in "$tests"
+    do
+        $(bazel info bazel-bin)/pkg/cmd/bazci/bazci_/bazci --config ci --config race test "$test" -- \
+                               --test_env=COCKROACH_LOGIC_TESTS_SKIP=true \
+                               --test_env=GOMAXPROCS=8
+    done
+done
+

docs/RFCS/20210722_on_update_and_row_rehoming.md

@@ -102,13 +102,33 @@ clause will not be re-evaluated. Applying the ON UPDATE to all rows is likely
 unexpected behavior, particularly when the ON UPDATE captures something unique
 about each row (region, last modified timestamp, etc.).
 
+### Same-column foreign key constraints
+
+If a foreign key column has both an ON UPDATE expression and a foreign key ON
+UPDATE reference action, it's unclear whether the ON UPDATE should apply after
+the reference action since its use would wipe any changes from the reference
+action. Because of this, we restrict ON UPDATE to be used only on columns which
+do not have a foreign key with an ON UPDATE action.
+
 ### Syntax
 
 Since this addition is a qualification on a column, it can be specified at
 create time with the following syntax:
 
 ```
-<column_name> <column_type> <other_qualifications> ON UPDATE SET <update_expr>
+<column_name> <column_type> <other_qualifications> ON UPDATE <update_expr>
+```
+
+Note: In earlier versions of this RFC, the syntax `ON UPDATE SET` was used. Upon
+further review, it was realized that it is impossible to disambiguate an ON
+UPDATE SET expression from an `ON UPDATE SET [NULL | DEFAULT]` on a foreign key
+constraint. For example, the following statement would have been ambiguous:
+
+```sql
+CREATE TABLE example(
+    p INT,
+    j INT REFERENCES other(j) ON UPDATE SET NULL
+)
 ```
 
 The ON UPDATE expression must be standalone i.e., it cannot reference other
@@ -125,7 +145,7 @@ value of 50 ON UPDATE would look like:
 CREATE TABLE inventories (
   product_id        INT,
   warehouse_id      INT,
-  quantity_on_hand  INT ON UPDATE SET 50,
+  quantity_on_hand  INT ON UPDATE 50,
   PRIMARY KEY (product_id, warehouse_id)
 );
 ```
@@ -134,7 +154,7 @@ This syntax also applies to `ALTER TABLE ALTER COLUMN`:
 
 ```sql
 -- Modifying an existing ON UPDATE expression or adding a new one
-ALTER TABLE <table_name> ALTER COLUMN <column_name> ON UPDATE SET <update_expr>
+ALTER TABLE <table_name> ALTER COLUMN <column_name> SET ON UPDATE <update_expr>
 -- Dropping an ON UPDATE expression
 ALTER TABLE <table_name> ALTER COLUMN <column_name> DROP ON UPDATE
 ```
@@ -143,7 +163,7 @@ Concretely with our inventories example, we have:
 
 ```sql
 -- Modifying the existing ON UPDATE expression to 50
-ALTER TABLE inventories ALTER COLUMN quantity_on_hand ON UPDATE SET 50
+ALTER TABLE inventories ALTER COLUMN quantity_on_hand SET ON UPDATE 50
 -- Dropping an ON UPDATE expression
 ALTER TABLE inventories ALTER COLUMN quantity_on_hand DROP ON UPDATE
 ```
@@ -163,7 +183,7 @@ SELECT quantity_on_hand FROM inventories;
 UPDATE inventories SET product_id = 2 WHERE warehouse_id = 1;
 SELECT quantity_on_hand FROM inventories;
 > 1
-ALTER TABLE inventories ALTER COLUMN quantity_on_hand ON UPDATE SET 50;
+ALTER TABLE inventories ALTER COLUMN quantity_on_hand SET ON UPDATE 50;
 UPDATE inventories SET product_id = 3 WHERE warehouse_id = 1;
 SELECT quantity_on_hand FROM inventories;
 > 50
@@ -205,7 +225,7 @@ CREATE TABLE test (
   p int,
   region crdb_internal_region
     DEFAULT default_to_database_primary_region(gateway_region())
-    ON UPDATE SET default_to_database_primary_region(gateway_region())
+    SET ON UPDATE default_to_database_primary_region(gateway_region())
 )
   LOCALITY REGIONAL BY ROW AS region;
 ```
@@ -274,7 +294,7 @@ accepts arbitrary expressions while the only MySQL usage of `ON UPDATE` is `ON
 UPDATE CURRENT_TIMESTAMP`. This proposal proposes a more general feature so that
 more workloads can be enabled. Because of the [special syntax
 form](https://dev.mysql.com/doc/refman/8.0/en/timestamp-initialization.html) for
-`current_timestamp()`, `ON UPDATE SET CURRENT_TIMESTAMP` is valid usage under
+`current_timestamp()`, `ON UPDATE CURRENT_TIMESTAMP` is valid usage under
 this proposal.
 
 This proposal is also distinct from the MySQL implementation in its interaction

pkg/cmd/dev/test.go

@@ -102,7 +102,7 @@ func (d *dev) runUnitTest(cmd *cobra.Command, pkgs []string) error {
 		args = append(args, fmt.Sprintf("--local_cpu_resources=%d", numCPUs))
 	}
 	if race {
-		args = append(args, "--@io_bazel_rules_go//go/config:race")
+		args = append(args, "--config=race")
 	}
 
 	for _, pkg := range pkgs {

pkg/cmd/github-pull-request-make/main.go

@@ -257,7 +257,7 @@ func main() {
 				// NB: stress and bazci are expected to be put in `PATH` by the caller.
 				args = append(args, "--run_under", fmt.Sprintf("stress -stderr -maxfails 1 -maxtime %s -p %d", duration, parallelism))
 				if target == "stressrace" {
-					args = append(args, "--@io_bazel_rules_go//go/config:race")
+					args = append(args, "--config=race")
 				}
 				cmd := exec.Command("bazci", args...)
 				cmd.Stdout = os.Stdout

pkg/kv/kvserver/replica_evaluate.go

@@ -269,7 +269,7 @@ func evaluateBatch(
 		// may carry a response transaction and in the case of WriteTooOldError
 		// (which is sometimes deferred) it is fully populated.
 		curResult, err := evaluateCommand(
-			ctx, idKey, index, readWriter, rec, ms, baHeader, args, reply, lul)
+			ctx, readWriter, rec, ms, baHeader, args, reply, lul)
 
 		if filter := rec.EvalKnobs().TestingPostEvalFilter; filter != nil {
 			filterArgs := kvserverbase.FilterArgs{
@@ -481,8 +481,6 @@ func evaluateBatch(
 // remaining for this batch (MaxInt64 for no limit).
 func evaluateCommand(
 	ctx context.Context,
-	raftCmdID kvserverbase.CmdIDKey,
-	index int,
 	readWriter storage.ReadWriter,
 	rec batcheval.EvalContext,
 	ms *enginepb.MVCCStats,

pkg/sql/colexec/colexecwindow/relative_rank_tmpl.go

@@ -309,7 +309,6 @@ func (r *_RELATIVE_RANK_STRINGOp) Init(ctx context.Context) {
 			DiskAcc:            r.diskAcc,
 		},
 	)
-	r.scratch = r.allocator.NewMemBatchWithFixedCapacity(r.inputTypes, coldata.BatchSize())
 	r.output = r.allocator.NewMemBatchWithFixedCapacity(append(r.inputTypes, types.Float), coldata.BatchSize())
 	// {{if .IsPercentRank}}
 	// All rank functions start counting from 1. Before we assign the rank to a
@@ -394,22 +393,12 @@ func (r *_RELATIVE_RANK_STRINGOp) Next() coldata.Batch {
 			// the input before we can proceed.
 			// {{end}}
 
-			sel := batch.Selection()
 			// First, we buffer up all of the tuples.
-			r.scratch.ResetInternalBatch()
-			r.allocator.PerformOperation(r.scratch.ColVecs(), func() {
-				for colIdx, vec := range r.scratch.ColVecs() {
-					vec.Copy(
-						coldata.SliceArgs{
-							Src:       batch.ColVec(colIdx),
-							Sel:       sel,
-							SrcEndIdx: n,
-						},
-					)
-				}
-				r.scratch.SetLength(n)
-			})
-			r.bufferedTuples.Enqueue(r.Ctx, r.scratch)
+			r.bufferedTuples.Enqueue(r.Ctx, batch)
+
+			// {{if or (.HasPartition) (.IsCumeDist)}}
+			sel := batch.Selection()
+			// {{end}}
 
 			// Then, we need to update the sizes of the partitions.
 			// {{if .HasPartition}}