Merge #65083 #65103 #65108 #65130

65083: kvserver: speed up TestRollbackSyncRangedIntentResolution r=tbg a=erikgrinaker

Release note: None

65103: schemaexpr: remove expr_filter.go r=mgartner a=mgartner

This commit removes the `pkg/sql/catalog/schemaexpr/expr_filter.go`
file. The `schemaexpr` package is intended to contain logic for dealing
with expressions defined in a schema, such as check constraints,
computed columns, and partial index predicates. The two exported
functions in the file, `RemapIVarsInTypedExpr` and `RunFilter` did not
fit this theme. They have been moved elsewhere.

Release note: None

65108: sql: minor fixes to fix panics related to type resolution and bad planner usage r=otan a=ajwerner

See individual commits. The first commit matters but does not leave one with a good feeling. The second one is more copacetic.

Relates to #64140. 

Fixes #64975.

Release note (bug fix): Fixed a bug which could cause a panic when
running a EXECUTE of a previously PREPARE'd statement with a REGCLASS,
REGTYPE parameter or a user-defined type argument after running BEGIN
AS OF SYSTEM TIME with an invalid timestamp.

Release note (bug fix): Fixed a bug which could cause a panic when issuing
a query referencing a user-defined type as a placeholder as the first operation
on a new connection.

65130: cli/flags.go: fix typo in comment r=rauchenstein a=knz

Thanks to @stevendanna for spotting this.

Co-authored-by: Erik Grinaker <grinaker@cockroachlabs.com>
Co-authored-by: Marcus Gartner <marcus@cockroachlabs.com>
Co-authored-by: Andrew Werner <awerner32@gmail.com>
Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>

pkg/cli/flags.go

@@ -999,7 +999,7 @@ const (
 // registerEnvVarDefault registers a deferred initialization of a flag
 // from an environment variable.
 // The caller is responsible for ensuring that the flagInfo has been
-// efined in the FlagSet already.
+// defined in the FlagSet already.
 func registerEnvVarDefault(f *pflag.FlagSet, flagInfo cliflags.FlagInfo) {
 	if flagInfo.EnvVar == "" {
 		return

pkg/kv/kvserver/intent_resolver_integration_test.go

@@ -194,9 +194,11 @@ func TestRollbackSyncRangedIntentResolution(t *testing.T) {
 	defer srv.Stopper().Stop(ctx)
 
 	txn := srv.DB().NewTxn(ctx, "test")
+	batch := txn.NewBatch()
 	for i := 0; i < 100000; i++ {
-		require.NoError(t, txn.Put(ctx, []byte(fmt.Sprintf("key%v", i)), []byte("value")))
+		batch.Put([]byte(fmt.Sprintf("key%v", i)), []byte("value"))
 	}
+	require.NoError(t, txn.Run(ctx, batch))
 	ctx, cancel := context.WithTimeout(ctx, 20*time.Second)
 	defer cancel()
 	require.NoError(t, txn.Rollback(ctx))

pkg/sql/catalog/schemaexpr/BUILD.bazel

@@ -10,7 +10,6 @@ go_library(
         "default_exprs.go",
         "doc.go",
         "expr.go",
-        "expr_filter.go",
         "partial_index.go",
         "select_name_resolution.go",
         "unique_contraint.go",

pkg/sql/conn_executor_exec.go

@@ -46,6 +46,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
 	"github.com/cockroachdb/cockroach/pkg/util/tracing"
 	"github.com/cockroachdb/errors"
+	"github.com/lib/pq/oid"
 )
 
 // execStmt executes one statement by dispatching according to the current
@@ -521,8 +522,9 @@ func (ex *connExecutor) execStmtInOpenState(
 			},
 			ex.generateID(),
 		)
+		var rawTypeHints []oid.Oid
 		if _, err := ex.addPreparedStmt(
-			ctx, name, prepStmt, typeHints, PreparedStatementOriginSQL,
+			ctx, name, prepStmt, typeHints, rawTypeHints, PreparedStatementOriginSQL,
 		); err != nil {
 			return makeErrEvent(err)
 		}
@@ -1106,7 +1108,17 @@ func (ex *connExecutor) beginTransactionTimestampsAndReadMode(
 	}
 	ex.statsCollector.reset(&ex.server.sqlStats, ex.appStats, &ex.phaseTimes)
 	p := &ex.planner
-	ex.resetPlanner(ctx, p, nil /* txn */, now)
+
+	// NB: this use of p.txn is totally bogus. The planner's txn should
+	// definitely be finalized at this point. We preserve it here because we
+	// need to make sure that the planner's txn is not made to be nil in the
+	// case of an error below. The planner's txn is never written to nil at
+	// any other point after the first prepare or exec has been run. We abuse
+	// this transaction in bind and some other contexts for resolving types and
+	// oids. Avoiding set this to nil side-steps a nil pointer panic but is still
+	// awful. Instead we ought to clear the planner state when we clear the reset
+	// the connExecutor in finishTxn.
+	ex.resetPlanner(ctx, p, p.txn, now)
 	ts, err := p.EvalAsOfTimestamp(ctx, asOf)
 	if err != nil {
 		return 0, time.Time{}, nil, err

pkg/sql/conn_executor_prepare.go

@@ -49,36 +49,13 @@ func (ex *connExecutor) execPrepare(
 		ex.deletePreparedStmt(ctx, "")
 	}
 
-	// If we were provided any type hints, attempt to resolve any user defined
-	// type OIDs into types.T's.
-	if parseCmd.TypeHints != nil {
-		for i := range parseCmd.TypeHints {
-			if parseCmd.TypeHints[i] == nil {
-				if i >= len(parseCmd.RawTypeHints) {
-					return retErr(
-						pgwirebase.NewProtocolViolationErrorf(
-							"expected %d arguments, got %d",
-							len(parseCmd.TypeHints),
-							len(parseCmd.RawTypeHints),
-						),
-					)
-				}
-				if types.IsOIDUserDefinedType(parseCmd.RawTypeHints[i]) {
-					var err error
-					parseCmd.TypeHints[i], err = ex.planner.ResolveTypeByOID(ctx, parseCmd.RawTypeHints[i])
-					if err != nil {
-						return retErr(err)
-					}
-				}
-			}
-		}
-	}
 	stmt := makeStatement(parseCmd.Statement, ex.generateID())
 	ps, err := ex.addPreparedStmt(
 		ctx,
 		parseCmd.Name,
 		stmt,
 		parseCmd.TypeHints,
+		parseCmd.RawTypeHints,
 		PreparedStatementOriginWire,
 	)
 	if err != nil {
@@ -115,20 +92,23 @@ func (ex *connExecutor) execPrepare(
 // also returned. It is illegal to call this when a statement with that name
 // already exists (even for anonymous prepared statements).
 //
-// placeholderHints are used to assist in inferring placeholder types.
+// placeholderHints are used to assist in inferring placeholder types. The
+// rawTypeHints are optional and represent OIDs indicated for the placeholders
+// coming from the client via the wire protocol.
 func (ex *connExecutor) addPreparedStmt(
 	ctx context.Context,
 	name string,
 	stmt Statement,
 	placeholderHints tree.PlaceholderTypes,
+	rawTypeHints []oid.Oid,
 	origin PreparedStatementOrigin,
 ) (*PreparedStatement, error) {
 	if _, ok := ex.extraTxnState.prepStmtsNamespace.prepStmts[name]; ok {
 		panic(errors.AssertionFailedf("prepared statement already exists: %q", name))
 	}
 
 	// Prepare the query. This completes the typing of placeholders.
-	prepared, err := ex.prepare(ctx, stmt, placeholderHints, origin)
+	prepared, err := ex.prepare(ctx, stmt, placeholderHints, rawTypeHints, origin)
 	if err != nil {
 		return nil, err
 	}
@@ -151,18 +131,11 @@ func (ex *connExecutor) prepare(
 	ctx context.Context,
 	stmt Statement,
 	placeholderHints tree.PlaceholderTypes,
+	rawTypeHints []oid.Oid,
 	origin PreparedStatementOrigin,
 ) (*PreparedStatement, error) {
-	if placeholderHints == nil {
-		placeholderHints = make(tree.PlaceholderTypes, stmt.NumPlaceholders)
-	}
 
 	prepared := &PreparedStatement{
-		PrepareMetadata: querycache.PrepareMetadata{
-			PlaceholderTypesInfo: tree.PlaceholderTypesInfo{
-				TypeHints: placeholderHints,
-			},
-		},
 		memAcc:   ex.sessionMon.MakeBoundAccount(),
 		refCount: 1,
 
@@ -176,16 +149,6 @@ func (ex *connExecutor) prepare(
 	if stmt.AST == nil {
 		return prepared, nil
 	}
-	prepared.Statement = stmt.Statement
-	prepared.AnonymizedStr = stmt.AnonymizedStr
-
-	// Point to the prepared state, which can be further populated during query
-	// preparation.
-	stmt.Prepared = prepared
-
-	if err := tree.ProcessPlaceholderAnnotations(&ex.planner.semaCtx, stmt.AST, placeholderHints); err != nil {
-		return nil, err
-	}
 
 	// Preparing needs a transaction because it needs to retrieve db/table
 	// descriptors for type checking. If we already have an open transaction for
@@ -209,6 +172,48 @@ func (ex *connExecutor) prepare(
 			// instrumentation.
 			ex.resetPlanner(ctx, p, txn, ex.server.cfg.Clock.PhysicalTime() /* stmtTS */)
 		}
+
+		if placeholderHints == nil {
+			placeholderHints = make(tree.PlaceholderTypes, stmt.NumPlaceholders)
+		} else if rawTypeHints != nil {
+			// If we were provided any type hints, attempt to resolve any user defined
+			// type OIDs into types.T's.
+			for i := range placeholderHints {
+				if placeholderHints[i] == nil {
+					if i >= len(rawTypeHints) {
+						return pgwirebase.NewProtocolViolationErrorf(
+							"expected %d arguments, got %d",
+							len(placeholderHints),
+							len(rawTypeHints),
+						)
+					}
+					if types.IsOIDUserDefinedType(rawTypeHints[i]) {
+						var err error
+						placeholderHints[i], err = ex.planner.ResolveTypeByOID(ctx, rawTypeHints[i])
+						if err != nil {
+							return err
+						}
+					}
+				}
+			}
+		}
+
+		prepared.PrepareMetadata = querycache.PrepareMetadata{
+			PlaceholderTypesInfo: tree.PlaceholderTypesInfo{
+				TypeHints: placeholderHints,
+			},
+		}
+		prepared.Statement = stmt.Statement
+		prepared.AnonymizedStr = stmt.AnonymizedStr
+
+		// Point to the prepared state, which can be further populated during query
+		// preparation.
+		stmt.Prepared = prepared
+
+		if err := tree.ProcessPlaceholderAnnotations(&ex.planner.semaCtx, stmt.AST, placeholderHints); err != nil {
+			return err
+		}
+
 		p.stmt = stmt
 		p.semaCtx.Annotations = tree.MakeAnnotations(stmt.NumAnnotations)
 		flags, err = ex.populatePrepared(ctx, txn, placeholderHints, p)

pkg/sql/execinfrapb/BUILD.bazel

@@ -27,7 +27,6 @@ go_library(
         "//pkg/sql/catalog/catalogkeys",
         "//pkg/sql/catalog/colinfo",
         "//pkg/sql/catalog/descpb",
-        "//pkg/sql/catalog/schemaexpr",
         "//pkg/sql/catalog/tabledesc",
         "//pkg/sql/parser",
         "//pkg/sql/pgwire/pgcode",

pkg/sql/execinfrapb/expr.go

@@ -13,7 +13,6 @@ package execinfrapb
 import (
 	"fmt"
 
-	"github.com/cockroachdb/cockroach/pkg/sql/catalog/schemaexpr"
 	"github.com/cockroachdb/cockroach/pkg/sql/parser"
 	"github.com/cockroachdb/cockroach/pkg/sql/rowenc"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/transform"
@@ -189,11 +188,25 @@ func (eh *ExprHelper) Init(
 func (eh *ExprHelper) EvalFilter(row rowenc.EncDatumRow) (bool, error) {
 	eh.Row = row
 	eh.evalCtx.PushIVarContainer(eh)
-	pass, err := schemaexpr.RunFilter(eh.Expr, eh.evalCtx)
+	pass, err := RunFilter(eh.Expr, eh.evalCtx)
 	eh.evalCtx.PopIVarContainer()
 	return pass, err
 }
 
+// RunFilter runs a filter expression and returns whether the filter passes.
+func RunFilter(filter tree.TypedExpr, evalCtx *tree.EvalContext) (bool, error) {
+	if filter == nil {
+		return true, nil
+	}
+
+	d, err := filter.Eval(evalCtx)
+	if err != nil {
+		return false, err
+	}
+
+	return d == tree.DBoolTrue, nil
+}
+
 // Eval - given a row - evaluates the wrapped expression and returns the
 // resulting datum. For example, given a row (1, 2, 3, 4, 5):
 //  '@2' would return '2'

pkg/sql/join_predicate.go

@@ -13,7 +13,7 @@ package sql
 import (
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
-	"github.com/cockroachdb/cockroach/pkg/sql/catalog/schemaexpr"
+	"github.com/cockroachdb/cockroach/pkg/sql/execinfrapb"
 	"github.com/cockroachdb/cockroach/pkg/sql/opt/exec"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/types"
@@ -135,7 +135,7 @@ func (p *joinPredicate) eval(ctx *tree.EvalContext, leftRow, rightRow tree.Datum
 		copy(p.curRow[:len(leftRow)], leftRow)
 		copy(p.curRow[len(leftRow):], rightRow)
 		ctx.PushIVarContainer(p.iVarHelper.Container())
-		pred, err := schemaexpr.RunFilter(p.onCond, ctx)
+		pred, err := execinfrapb.RunFilter(p.onCond, ctx)
 		ctx.PopIVarContainer()
 		return pred, err
 	}