Merge #39804 #39818

39804: opt: add FK benchmarks r=RaduBerinde a=RaduBerinde

Adding simple insert benchmarks comparing no FKs with the old FK path
and the new one.

The results are below. Note that we haven't yet optimized the constant
input case (we can remove the buffer / scan nodes).

Command line:
```
GOMAXPROCS=1 make bench PKG=./pkg/sql/opt/bench BENCHTIMEOUT=30m BENCHES='BenchmarkFKInsert/' TESTFLAGS='-logtostderr NONE -benchtime=20000x -count 5'
```

```
name                                time/op
FKInsert/SingleRow/None              609µs ± 4%
FKInsert/SingleRow/Old               676µs ± 5%
FKInsert/SingleRow/New               961µs ± 1%
FKInsert/MultiRowSingleParent/None  1.37ms ± 1%
FKInsert/MultiRowSingleParent/Old   2.14ms ± 1%
FKInsert/MultiRowSingleParent/New   2.11ms ± 2%
FKInsert/MultiRowMultiParent/None   1.62ms ± 2%
FKInsert/MultiRowMultiParent/Old    2.89ms ± 1%
FKInsert/MultiRowMultiParent/New    2.97ms ± 1%
```

Release note: None

39818: opt: nil presentation should not equal the 0-column presentation r=RaduBerinde a=RaduBerinde

#### sql: enhance apply execbuilder error

We have seen a series of bugs where the execbuilder hits an assertion
error inside Apply. This change adds a detail to the error containing
the formatted opt tree.

Release note: None

#### opt: nil presentation should not equal the 0-column presentation

The nil presentation means that we want all columns in no particular
order, whereas the 0-column presentation means that we don't need any
of the columns (e.g. EXISTS subqueries). Only the nil presentation
is `Any()`. But `Equals()` was incorrectly treating these as equal.
This confused the interner and a nil presentation became a 0-column
presentation which led to some internal errors in Apply.

For completeness, we also fix `HashPhysProps` to differentiate
between these two, but note that this isn't required for correctness.

Fixes #39435.

Release note (bug fix): Fixed internal errors generated during
execution of some complicated cases of correlated subqueries.


Co-authored-by: Radu Berinde <radu@cockroachlabs.com>

pkg/sql/apply_join.go

@@ -266,6 +266,13 @@ func (a *applyJoinNode) Next(params runParams) (bool, error) {
 		eb.DisableTelemetry()
 		p, err := eb.Build()
 		if err != nil {
+			if errors.IsAssertionFailure(err) {
+				// Enhance the error with the EXPLAIN (OPT, VERBOSE) of the inner
+				// expression.
+				fmtFlags := memo.ExprFmtHideQualifications | memo.ExprFmtHideScalars | memo.ExprFmtHideTypes
+				explainOpt := memo.FormatExpr(newRightSide, fmtFlags, nil /* catalog */)
+				err = errors.WithDetailf(err, "newRightSide:\n%s", explainOpt)
+			}
 			return false, err
 		}
 		plan := p.(*planTop)

pkg/sql/opt/bench/fk_test.go

@@ -0,0 +1,111 @@
+// Copyright 2019 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package bench
+
+import (
+	"context"
+	"fmt"
+	"strings"
+	"testing"
+
+	"github.com/cockroachdb/cockroach/pkg/base"
+	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
+	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
+)
+
+func runFKBench(
+	b *testing.B,
+	setup func(b *testing.B, r *sqlutils.SQLRunner, setupFKs bool),
+	run func(b *testing.B, r *sqlutils.SQLRunner),
+) {
+	configs := []struct {
+		name     string
+		setupFKs bool
+		optFKOn  bool
+	}{
+		{name: "None", setupFKs: false},
+		{name: "Old", setupFKs: true, optFKOn: false},
+		{name: "New", setupFKs: true, optFKOn: true},
+	}
+
+	for _, cfg := range configs {
+		b.Run(cfg.name, func(b *testing.B) {
+			s, db, _ := serverutils.StartServer(b, base.TestServerArgs{})
+			defer s.Stopper().Stop(context.TODO())
+			r := sqlutils.MakeSQLRunner(db)
+			// Don't let auto stats interfere with the test. Stock stats are
+			// sufficient to get the right plans (i.e. lookup join).
+			r.Exec(b, "SET CLUSTER SETTING sql.stats.automatic_collection.enabled = false")
+			r.Exec(b, fmt.Sprintf("SET experimental_optimizer_foreign_keys = %v", cfg.optFKOn))
+			setup(b, r, cfg.setupFKs)
+			b.ResetTimer()
+			run(b, r)
+		})
+	}
+}
+
+func BenchmarkFKInsert(b *testing.B) {
+	const parentRows = 1000
+	setup := func(b *testing.B, r *sqlutils.SQLRunner, setupFKs bool) {
+		r.Exec(b, "CREATE TABLE child (k int primary key, p int)")
+		r.Exec(b, "CREATE TABLE parent (p int primary key, data int)")
+
+		if setupFKs {
+			r.Exec(b, "ALTER TABLE child ADD CONSTRAINT fk FOREIGN KEY (p) REFERENCES parent(p)")
+		} else {
+			// Create the index on p manually so it's a more fair comparison.
+			r.Exec(b, "CREATE INDEX idx ON child(p)")
+		}
+
+		r.Exec(b, fmt.Sprintf(
+			"INSERT INTO parent SELECT i, i FROM generate_series(0,%d) AS g(i)", parentRows-1,
+		))
+	}
+
+	b.Run("SingleRow", func(b *testing.B) {
+		runFKBench(b, setup, func(b *testing.B, r *sqlutils.SQLRunner) {
+			for i := 0; i < b.N; i++ {
+				r.Exec(b, fmt.Sprintf("INSERT INTO child VALUES (%d, %d)", i, i%parentRows))
+			}
+		})
+	})
+
+	const batch = 20
+	b.Run("MultiRowSingleParent", func(b *testing.B) {
+		runFKBench(b, setup, func(b *testing.B, r *sqlutils.SQLRunner) {
+			k := 0
+			for i := 0; i < b.N; i++ {
+				// All rows in the batch reference the same parent value.
+				parent := i % parentRows
+				vals := make([]string, batch)
+				for j := range vals {
+					vals[j] = fmt.Sprintf("(%d, %d)", k, parent)
+					k++
+				}
+				r.Exec(b, fmt.Sprintf("INSERT INTO child VALUES %s", strings.Join(vals, ",")))
+			}
+		})
+	})
+
+	b.Run("MultiRowMultiParent", func(b *testing.B) {
+		runFKBench(b, setup, func(b *testing.B, r *sqlutils.SQLRunner) {
+			k := 0
+			for i := 0; i < b.N; i++ {
+				vals := make([]string, batch)
+				for j := range vals {
+					vals[j] = fmt.Sprintf("(%d, %d)", k, k%parentRows)
+					k++
+				}
+				r.Exec(b, fmt.Sprintf("INSERT INTO child VALUES %s", strings.Join(vals, ",")))
+			}
+		})
+	})
+}

pkg/sql/opt/memo/interner.go

@@ -522,10 +522,14 @@ func (h *hasher) HashTupleOrdinal(val TupleOrdinal) {
 }
 
 func (h *hasher) HashPhysProps(val *physical.Required) {
-	for i := range val.Presentation {
-		col := &val.Presentation[i]
-		h.HashString(col.Alias)
-		h.HashColumnID(col.ID)
+	// Note: the Any presentation is not the same as the 0-column presentation.
+	if !val.Presentation.Any() {
+		h.HashInt(len(val.Presentation))
+		for i := range val.Presentation {
+			col := &val.Presentation[i]
+			h.HashString(col.Alias)
+			h.HashColumnID(col.ID)
+		}
 	}
 	h.HashOrderingChoice(val.Ordering)
 }

pkg/sql/opt/props/physical/required.go

@@ -115,6 +115,10 @@ func (p Presentation) Any() bool {
 // Equals returns true iff this presentation exactly matches the given
 // presentation.
 func (p Presentation) Equals(rhs Presentation) bool {
+	// The 0 column presentation is not the same as the nil presentation.
+	if p.Any() != rhs.Any() {
+		return false
+	}
 	if len(p) != len(rhs) {
 		return false
 	}

pkg/sql/opt/props/physical/required_test.go

@@ -42,10 +42,18 @@ func TestRequiredProps(t *testing.T) {
 		t.Error("presentation should equal itself")
 	}
 
-	if presentation.Equals(physical.Presentation{}) {
+	if presentation.Equals(physical.Presentation(nil)) {
 		t.Error("presentation should not equal the empty presentation")
 	}
 
+	if presentation.Equals(physical.Presentation{}) {
+		t.Error("presentation should not equal the 0 column presentation")
+	}
+
+	if (physical.Presentation{}).Equals(physical.Presentation(nil)) {
+		t.Error("0 column presentation should not equal the empty presentation")
+	}
+
 	// Add ordering props.
 	ordering := physical.ParseOrderingChoice("+1,+5")
 	phys.Ordering = ordering