sql/opt: avoid swallowing TransactionAbortedErrors

An optimizer code path could, in rare cases, fail to propagate a
transaction aborted error. This proved disastrous as, due to a footgun
in our transaction API (cockroachdb#22615), swallowing a transaction aborted error
results in proceeding with a brand new transaction that has no knowledge
of the earlier operations performed on the original transaction.

This presented as a rare and confusing bug in splits, as the split
transaction uses an internal executor. The internal executor would
occasionally silently return a new transaction that only had half of the
necessary operations performed on it, and committing that partial
transaction would result in a "range does not match splits" error.

Fixes cockroachdb#32784.

Release note: None

pkg/sql/logictest/testdata/logic_test/prepare

@@ -739,7 +739,8 @@ EXECUTE change_view
 statement ok
 ALTER VIEW otherview RENAME TO otherview2
 
-query error pq: relation "otherview" does not exist
+# HP and CBO return slightly different errors, so accept both.
+query error pq: relation "(otherdb.public.)?otherview" does not exist
 EXECUTE change_view
 
 statement ok

pkg/sql/opt/memo/memo.go

@@ -247,29 +247,36 @@ func (m *Memo) HasPlaceholders() bool {
 //   5. Data source privileges: current user may no longer have access to one or
 //      more data sources.
 //
-func (m *Memo) IsStale(ctx context.Context, evalCtx *tree.EvalContext, catalog cat.Catalog) bool {
+// This function cannot swallow errors and return only a boolean, as it may
+// perform KV operations on behalf of the transaction associated with the
+// provided catalog, and those errors are required to be propagated.
+func (m *Memo) IsStale(
+	ctx context.Context, evalCtx *tree.EvalContext, catalog cat.Catalog,
+) (bool, error) {
 	// Memo is stale if the current database has changed.
 	if m.dbName != evalCtx.SessionData.Database {
-		return true
+		return true, nil
 	}
 
 	// Memo is stale if the search path has changed.
 	if !m.searchPath.Equals(&evalCtx.SessionData.SearchPath) {
-		return true
+		return true, nil
 	}
 
 	// Memo is stale if the location has changed.
 	if m.locName != evalCtx.GetLocation().String() {
-		return true
+		return true, nil
 	}
 
 	// Memo is stale if the fingerprint of any data source in the memo's metadata
 	// has changed, or if the current user no longer has sufficient privilege to
 	// access the data source.
-	if !m.Metadata().CheckDependencies(ctx, catalog) {
-		return true
+	if depsUpToDate, err := m.Metadata().CheckDependencies(ctx, catalog); err != nil {
+		return false, err
+	} else if !depsUpToDate {
+		return true, nil
 	}
-	return false
+	return false, nil
 }
 
 // InternPhysicalProps adds the given physical props to the memo if they haven't

pkg/sql/opt/memo/memo_test.go

@@ -22,12 +22,13 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/sql/opt/memo"
 	"github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder"
-	"github.com/cockroachdb/cockroach/pkg/sql/opt/testutils"
+	opttestutils "github.com/cockroachdb/cockroach/pkg/sql/opt/testutils"
 	"github.com/cockroachdb/cockroach/pkg/sql/opt/testutils/testcat"
 	"github.com/cockroachdb/cockroach/pkg/sql/opt/xform"
 	"github.com/cockroachdb/cockroach/pkg/sql/parser"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
+	testutils "github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/datadriven"
 )
 
@@ -121,31 +122,41 @@ func TestMemoIsStale(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	if o.Memo().IsStale(ctx, &evalCtx, catalog) {
+	if isStale, err := o.Memo().IsStale(ctx, &evalCtx, catalog); err != nil {
+		t.Fatal(err)
+	} else if isStale {
 		t.Errorf("memo should not be stale")
 	}
 
 	// Stale current database.
 	evalCtx.SessionData.Database = "newdb"
-	if !o.Memo().IsStale(ctx, &evalCtx, catalog) {
+	if isStale, err := o.Memo().IsStale(ctx, &evalCtx, catalog); err != nil {
+		t.Fatal(err)
+	} else if !isStale {
 		t.Errorf("expected stale current database")
 	}
 	evalCtx.SessionData.Database = "t"
 
 	// Stale search path.
 	evalCtx.SessionData.SearchPath = sessiondata.SearchPath{}
-	if !o.Memo().IsStale(ctx, &evalCtx, catalog) {
+	if isStale, err := o.Memo().IsStale(ctx, &evalCtx, catalog); err != nil {
+		t.Fatal(err)
+	} else if !isStale {
 		t.Errorf("expected stale search path")
 	}
 	evalCtx.SessionData.SearchPath = sessiondata.MakeSearchPath([]string{"path1", "path2"})
-	if o.Memo().IsStale(ctx, &evalCtx, catalog) {
+	if isStale, err := o.Memo().IsStale(ctx, &evalCtx, catalog); err != nil {
+		t.Fatal(err)
+	} else if isStale {
 		t.Errorf("memo should not be stale")
 	}
 	evalCtx.SessionData.SearchPath = sessiondata.MakeSearchPath(searchPath)
 
 	// Stale location.
 	evalCtx.SessionData.DataConversion.Location = time.FixedZone("PST", -8*60*60)
-	if !o.Memo().IsStale(ctx, &evalCtx, catalog) {
+	if isStale, err := o.Memo().IsStale(ctx, &evalCtx, catalog); err != nil {
+		t.Fatal(err)
+	} else if !isStale {
 		t.Errorf("expected stale location")
 	}
 	evalCtx.SessionData.DataConversion.Location = time.UTC
@@ -159,7 +170,9 @@ func TestMemoIsStale(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	if !o.Memo().IsStale(ctx, &evalCtx, catalog) {
+	if isStale, err := o.Memo().IsStale(ctx, &evalCtx, catalog); err != nil {
+		t.Fatal(err)
+	} else if !isStale {
 		t.Errorf("expected stale schema")
 	}
 	catalog = testcat.New()
@@ -168,13 +181,16 @@ func TestMemoIsStale(t *testing.T) {
 
 	// User no longer has access to view.
 	catalog.View(tree.NewTableName("t", "abcview")).Revoked = true
-	if !o.Memo().IsStale(ctx, &evalCtx, catalog) {
-		t.Errorf("expected user not to have SELECT privilege on view")
+	_, err = o.Memo().IsStale(ctx, &evalCtx, catalog)
+	if exp := "user does not have privilege"; !testutils.IsError(err, exp) {
+		t.Fatalf("expected %q error, but got %+v", exp, err)
 	}
 	catalog.View(tree.NewTableName("t", "abcview")).Revoked = false
 
 	// Ensure that memo is not stale after restoring to original state.
-	if o.Memo().IsStale(ctx, &evalCtx, catalog) {
+	if isStale, err := o.Memo().IsStale(ctx, &evalCtx, catalog); err != nil {
+		t.Fatal(err)
+	} else if isStale {
 		t.Errorf("memo should not be stale")
 	}
 }
@@ -190,7 +206,7 @@ func runDataDrivenTest(t *testing.T, path string, fmtFlags memo.ExprFmtFlags) {
 	datadriven.Walk(t, path, func(t *testing.T, path string) {
 		catalog := testcat.New()
 		datadriven.RunTest(t, path, func(d *datadriven.TestData) string {
-			tester := testutils.NewOptTester(catalog, d.Input)
+			tester := opttestutils.NewOptTester(catalog, d.Input)
 			tester.Flags.ExprFormat = fmtFlags
 			return tester.RunCommand(t, d)
 		})

pkg/sql/opt/metadata.go

@@ -119,17 +119,21 @@ func (md *Metadata) AddDependency(ds cat.DataSource, priv privilege.Kind) {
 // in order to check that the fully qualified data source names still resolve to
 // the same version of the same data source, and that the user still has
 // sufficient privileges to access the data source.
-func (md *Metadata) CheckDependencies(ctx context.Context, catalog cat.Catalog) bool {
+//
+// This function cannot swallow errors and return only a boolean, as it may
+// perform KV operations on behalf of the transaction associated with the
+// provided catalog, and those errors are required to be propagated.
+func (md *Metadata) CheckDependencies(ctx context.Context, catalog cat.Catalog) (bool, error) {
 	for dep, privs := range md.deps {
 		ds, err := catalog.ResolveDataSource(ctx, dep.Name())
 		if err != nil {
-			return false
+			return false, err
 		}
 		if dep.ID() != ds.ID() {
-			return false
+			return false, nil
 		}
 		if dep.Version() != ds.Version() {
-			return false
+			return false, nil
 		}
 
 		for privs != 0 {
@@ -140,15 +144,15 @@ func (md *Metadata) CheckDependencies(ctx context.Context, catalog cat.Catalog)
 			priv := privilege.Kind(bits.TrailingZeros32(uint32(privs)))
 			if priv != 0 {
 				if err = catalog.CheckPrivilege(ctx, ds, priv); err != nil {
-					return false
+					return false, err
 				}
 			}
 
 			// Set the just-handled privilege bit to zero and look for next.
 			privs &= ^(1 << priv)
 		}
 	}
-	return true
+	return true, nil
 }
 
 // AddTable indexes a new reference to a table within the query. Separate

pkg/sql/plan.go

@@ -439,7 +439,9 @@ func (p *planner) makeOptimizerPlan(ctx context.Context, stmt Statement) (planFl
 
 		// If the prepared memo has been invalidated by schema or other changes,
 		// re-prepare it.
-		if stmt.Prepared.Memo.IsStale(ctx, p.EvalContext(), &catalog) {
+		if isStale, err := stmt.Prepared.Memo.IsStale(ctx, p.EvalContext(), &catalog); err != nil {
+			return 0, err
+		} else if isStale {
 			stmt.Prepared.Memo, err = p.prepareMemo(ctx, &catalog, stmt)
 			if err != nil {
 				return 0, err
@@ -451,7 +453,9 @@ func (p *planner) makeOptimizerPlan(ctx context.Context, stmt Statement) (planFl
 		// Consult the query cache.
 		cachedData, ok := p.execCfg.QueryCache.Find(stmt.SQL)
 		if ok {
-			if cachedData.Memo.IsStale(ctx, p.EvalContext(), &catalog) {
+			if isStale, err := cachedData.Memo.IsStale(ctx, p.EvalContext(), &catalog); err != nil {
+				return 0, err
+			} else if isStale {
 				cachedData.Memo, err = p.prepareMemo(ctx, &catalog, stmt)
 				if err != nil {
 					return 0, err