diff --git a/pkg/cli/flags.go b/pkg/cli/flags.go index cb493ec2672b..bcc467f808be 100644 --- a/pkg/cli/flags.go +++ b/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 diff --git a/pkg/kv/kvserver/intent_resolver_integration_test.go b/pkg/kv/kvserver/intent_resolver_integration_test.go index 2cb0f8c0ee72..75229f884577 100644 --- a/pkg/kv/kvserver/intent_resolver_integration_test.go +++ b/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)) diff --git a/pkg/sql/catalog/schemaexpr/BUILD.bazel b/pkg/sql/catalog/schemaexpr/BUILD.bazel index 387f57310221..0c7344fa9e38 100644 --- a/pkg/sql/catalog/schemaexpr/BUILD.bazel +++ b/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", diff --git a/pkg/sql/catalog/schemaexpr/expr_filter.go b/pkg/sql/catalog/schemaexpr/expr_filter.go deleted file mode 100644 index 8d3181532d9d..000000000000 --- a/pkg/sql/catalog/schemaexpr/expr_filter.go +++ /dev/null @@ -1,52 +0,0 @@ -// Copyright 2016 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 schemaexpr - -import "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" - -// 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 -} - -type ivarRemapper struct { - indexVarMap []int -} - -var _ tree.Visitor = &ivarRemapper{} - -func (v *ivarRemapper) VisitPre(expr tree.Expr) (recurse bool, newExpr tree.Expr) { - if ivar, ok := expr.(*tree.IndexedVar); ok { - newIvar := *ivar - newIvar.Idx = v.indexVarMap[ivar.Idx] - return false, &newIvar - } - return true, expr -} - -func (*ivarRemapper) VisitPost(expr tree.Expr) tree.Expr { return expr } - -// RemapIVarsInTypedExpr remaps tree.IndexedVars in expr using indexVarMap. -// Note that a new expression is returned. -func RemapIVarsInTypedExpr(expr tree.TypedExpr, indexVarMap []int) tree.TypedExpr { - v := &ivarRemapper{indexVarMap: indexVarMap} - newExpr, _ := tree.WalkExpr(v, expr) - return newExpr.(tree.TypedExpr) -} diff --git a/pkg/sql/conn_executor_exec.go b/pkg/sql/conn_executor_exec.go index 06ef29d7ddeb..a44c7ce019e1 100644 --- a/pkg/sql/conn_executor_exec.go +++ b/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 diff --git a/pkg/sql/conn_executor_prepare.go b/pkg/sql/conn_executor_prepare.go index c4f4c74094c6..e7ccfbca2e6e 100644 --- a/pkg/sql/conn_executor_prepare.go +++ b/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,12 +92,15 @@ 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 { @@ -128,7 +108,7 @@ func (ex *connExecutor) addPreparedStmt( } // 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) diff --git a/pkg/sql/execinfrapb/BUILD.bazel b/pkg/sql/execinfrapb/BUILD.bazel index 2d5deaa75516..63a52b616656 100644 --- a/pkg/sql/execinfrapb/BUILD.bazel +++ b/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", diff --git a/pkg/sql/execinfrapb/expr.go b/pkg/sql/execinfrapb/expr.go index 7f6d9d50905c..c6358128c1d4 100644 --- a/pkg/sql/execinfrapb/expr.go +++ b/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' diff --git a/pkg/sql/join_predicate.go b/pkg/sql/join_predicate.go index 5c1a0580d191..944932ab6fb2 100644 --- a/pkg/sql/join_predicate.go +++ b/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 } diff --git a/pkg/sql/pgwire/testdata/pgtest/bind_and_resolve b/pkg/sql/pgwire/testdata/pgtest/bind_and_resolve new file mode 100644 index 000000000000..cdf095a8e092 --- /dev/null +++ b/pkg/sql/pgwire/testdata/pgtest/bind_and_resolve @@ -0,0 +1,70 @@ +# Test binding calls which reference names via either types or oid casts work +# after a bizarre turn of events whereby we used to set the planner txn to nil. +# The long and the short of it is that we totally abuse the planner when +# performing BIND and the fact that it continues to carry a transaction that +# does not correspond to the connExecutor's transaction is, generally, a big +# problem. Before the changes made in the commit adding this test, this test +# would result in a nil pointer panic. + +send +Query {"String": "CREATE TABLE t (a INT PRIMARY KEY)"} +---- + +until +ReadyForQuery +---- +{"Type":"CommandComplete","CommandTag":"CREATE TABLE"} +{"Type":"ReadyForQuery","TxStatus":"I"} + + +# 83 = ASCII 'S' for Statement +# 84 = ASCII 'T' +# ParameterFormatCodes = [0] for text format +send +Parse {"Name": "s7", "Query": "SELECT $1::REGCLASS::INT8"} +Describe {"ObjectType": 83, "Name": "s7"} +Sync +---- + +until +ReadyForQuery +---- +{"Type":"ParseComplete"} +{"Type":"ParameterDescription","ParameterOIDs":[2205]} +{"Type":"RowDescription","Fields":[{"Name":"int8","TableOID":0,"TableAttributeNumber":0,"DataTypeOID":20,"DataTypeSize":8,"TypeModifier":-1,"Format":0}]} +{"Type":"ReadyForQuery","TxStatus":"I"} + +# The below incantation used to trigger a code path which would nil the +# planner transaction but never set it. This was pretty much the only +# way you could do such a thing. + +send +Query {"String": "BEGIN AS OF SYSTEM TIME '1s'"} +Sync +---- + + +# TODO(ajwerner): Why are there two ReadyForQuery? + +until +ErrorResponse +ReadyForQuery +ReadyForQuery +---- +{"Type":"ErrorResponse","Code":"XXUUU"} +{"Type":"ReadyForQuery","TxStatus":"I"} +{"Type":"ReadyForQuery","TxStatus":"I"} + +send +Bind {"DestinationPortal": "p7", "PreparedStatement": "s7", "ParameterFormatCodes": [0], "Parameters": [[84]]} +Execute {"Portal": "p7"} +Sync +---- + +until +ReadyForQuery +---- +{"Type":"BindComplete"} +{"Type":"DataRow","Values":[{"text":"52"}]} +{"Type":"CommandComplete","CommandTag":"SELECT 1"} +{"Type":"ReadyForQuery","TxStatus":"I"} diff --git a/pkg/sql/pgwire/testdata/pgtest/enum b/pkg/sql/pgwire/testdata/pgtest/enum index 7af403abf7c2..41d2bce0140f 100644 --- a/pkg/sql/pgwire/testdata/pgtest/enum +++ b/pkg/sql/pgwire/testdata/pgtest/enum @@ -1,5 +1,23 @@ # This test verifies some of the pgwire encoding process for ENUMs. +# Demonstrate that attempting to prepare a query which hints a user-defined +# type as the very first thing on a new connection does not cause a panic. +# Prior to the change which introduced this test logic, the below prepare +# would hit a nil-pointer panic. + +send crdb_only +Parse {"Name": "s1", "Query": "SELECT $1", "ParameterOIDs": [100052]} +Sync +---- + +until crdb_only +ErrorResponse +ReadyForQuery +---- +{"Type":"ErrorResponse","Code":"42704"} +{"Type":"ReadyForQuery","TxStatus":"I"} + + # Prepare the environment. send noncrdb_only Query {"String": "DROP TYPE IF EXISTS te CASCADE"} diff --git a/pkg/sql/physicalplan/BUILD.bazel b/pkg/sql/physicalplan/BUILD.bazel index 107840fc7d7d..c8175ca68aab 100644 --- a/pkg/sql/physicalplan/BUILD.bazel +++ b/pkg/sql/physicalplan/BUILD.bazel @@ -20,7 +20,6 @@ go_library( "//pkg/rpc", "//pkg/settings/cluster", "//pkg/sql/catalog/colinfo", - "//pkg/sql/catalog/schemaexpr", "//pkg/sql/execinfra", "//pkg/sql/execinfrapb", "//pkg/sql/physicalplan/replicaoracle", diff --git a/pkg/sql/physicalplan/expression.go b/pkg/sql/physicalplan/expression.go index 554c07698813..be59c45b415c 100644 --- a/pkg/sql/physicalplan/expression.go +++ b/pkg/sql/physicalplan/expression.go @@ -14,7 +14,6 @@ package physicalplan import ( - "github.com/cockroachdb/cockroach/pkg/sql/catalog/schemaexpr" "github.com/cockroachdb/cockroach/pkg/sql/execinfrapb" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/types" @@ -88,7 +87,7 @@ func MakeExpression( if indexVarMap != nil { // Remap our indexed vars. - expr = schemaexpr.RemapIVarsInTypedExpr(expr, indexVarMap) + expr = remapIVarsInTypedExpr(expr, indexVarMap) } expression := execinfrapb.Expression{LocalExpr: expr} if ctx.IsLocal() { @@ -132,3 +131,28 @@ func (e *evalAndReplaceSubqueryVisitor) VisitPre(expr tree.Expr) (bool, tree.Exp } func (evalAndReplaceSubqueryVisitor) VisitPost(expr tree.Expr) tree.Expr { return expr } + +// remapIVarsInTypedExpr remaps tree.IndexedVars in expr using indexVarMap. +// Note that a new expression is returned. +func remapIVarsInTypedExpr(expr tree.TypedExpr, indexVarMap []int) tree.TypedExpr { + v := &ivarRemapper{indexVarMap: indexVarMap} + newExpr, _ := tree.WalkExpr(v, expr) + return newExpr.(tree.TypedExpr) +} + +type ivarRemapper struct { + indexVarMap []int +} + +var _ tree.Visitor = &ivarRemapper{} + +func (v *ivarRemapper) VisitPre(expr tree.Expr) (recurse bool, newExpr tree.Expr) { + if ivar, ok := expr.(*tree.IndexedVar); ok { + newIvar := *ivar + newIvar.Idx = v.indexVarMap[ivar.Idx] + return false, &newIvar + } + return true, expr +} + +func (*ivarRemapper) VisitPost(expr tree.Expr) tree.Expr { return expr }