Skip to content

Commit

Permalink
Merge #89542
Browse files Browse the repository at this point in the history
89542: opt: do not plan insert fast path if expression contains UDF r=mgartner a=mgartner

In general, it is not safe to perform the INSERT fast path if the VALUES clause invokes a UDF. The INSERT fast path always performs the FK checks before the INSERT. A UDF might mutate other tables (although we don't currently support this), so the FK checks must be performed after the INSERT is complete.

Release note: None

Co-authored-by: Marcus Gartner <[email protected]>
  • Loading branch information
craig[bot] and mgartner committed Oct 7, 2022
2 parents 308fbcb + c004da8 commit 44c714c
Show file tree
Hide file tree
Showing 6 changed files with 67 additions and 8 deletions.
6 changes: 4 additions & 2 deletions pkg/sql/opt/exec/execbuilder/mutation.go
Original file line number Diff line number Diff line change
Expand Up @@ -146,8 +146,10 @@ func (b *Builder) tryBuildFastPathInsert(ins *memo.InsertExpr) (_ execPlan, ok b
// that we send, not a number of rows. We use this as a guideline only,
// and there is no guarantee that we won't produce a bigger batch.)
values, ok := ins.Input.(*memo.ValuesExpr)
// TODO(mgartner): Prevent fast path if there is a UDF invocation.
if !ok || values.ChildCount() > mutations.MaxBatchSize(false /* forceProductionMaxBatchSize */) || values.Relational().HasSubquery {
if !ok ||
values.ChildCount() > mutations.MaxBatchSize(false /* forceProductionMaxBatchSize */) ||
values.Relational().HasSubquery ||
values.Relational().HasUDF {
return execPlan{}, false, nil
}

Expand Down
51 changes: 51 additions & 0 deletions pkg/sql/opt/exec/execbuilder/testdata/insert
Original file line number Diff line number Diff line change
Expand Up @@ -534,6 +534,57 @@ vectorized: true
statement ok
ROLLBACK

# ------------------------------------------------------------------------------
# Insert fast path.
# ------------------------------------------------------------------------------

# Do not plan insert fast path when VALUES clause has a subquery.
# statement ok
query T
EXPLAIN INSERT INTO kv VALUES (1, (SELECT v FROM kv))
----
distribution: local
vectorized: true
·
• root
├── • insert
│ │ into: kv(k, v)
│ │ auto commit
│ │
│ └── • values
│ size: 2 columns, 1 row
└── • subquery
│ id: @S1
│ original sql: (SELECT v FROM kv)
│ exec mode: one row
└── • max1row
│ estimated row count: 1
└── • scan
missing stats
table: kv@kv_pkey
spans: FULL SCAN

statement ok
CREATE FUNCTION foo() RETURNS VARCHAR LANGUAGE SQL AS 'SELECT v FROM kv'

# Do not plan insert fast path when VALUES clause invokes a UDF.
query T
EXPLAIN INSERT INTO kv VALUES (1, foo())
----
distribution: local
vectorized: true
·
• insert
│ into: kv(k, v)
│ auto commit
└── • values
size: 2 columns, 1 row

# Regression test for #35564: make sure we use the Insert's input required
# ordering for the internal projection.

Expand Down
3 changes: 3 additions & 0 deletions pkg/sql/opt/memo/expr_format.go
Original file line number Diff line number Diff line change
Expand Up @@ -1122,6 +1122,9 @@ func (f *ExprFmtCtx) scalarPropsStrings(scalar opt.ScalarExpr) []string {
} else if scalarProps.HasSubquery {
emitProp("subquery")
}
if scalarProps.HasUDF {
emitProp("udf")
}
}

if !f.HasFlags(ExprFmtHideConstraints) {
Expand Down
1 change: 1 addition & 0 deletions pkg/sql/opt/memo/logical_props_builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -1657,6 +1657,7 @@ func BuildSharedProps(e opt.Expr, shared *props.Shared, evalCtx *eval.Context) {
shared.VolatilitySet.Add(volatility)

case *UDFExpr:
shared.HasUDF = true
shared.VolatilitySet.Add(t.Volatility)

default:
Expand Down
8 changes: 4 additions & 4 deletions pkg/sql/opt/memo/testdata/logprops/udf
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ project
│ ├── prune: (1-4)
│ └── interesting orderings: (+1)
└── projections
└── plus [as="?column?":6, type=int, outer=(1), volatile]
└── plus [as="?column?":6, type=int, outer=(1), volatile, udf]
├── variable: a:1 [type=int]
└── udf: fn_volatile [type=int]
└── body
Expand Down Expand Up @@ -80,7 +80,7 @@ project
│ ├── prune: (1-4)
│ └── interesting orderings: (+1)
└── filters
└── eq [type=bool, outer=(2), immutable, constraints=(/2: (/NULL - ]), fd=()-->(2)]
└── eq [type=bool, outer=(2), immutable, udf, constraints=(/2: (/NULL - ]), fd=()-->(2)]
├── variable: b:2 [type=int]
└── udf: fn_immutable [type=int]
└── body
Expand Down Expand Up @@ -125,7 +125,7 @@ project
│ ├── prune: (1-4)
│ └── interesting orderings: (+1)
└── filters
└── eq [type=bool, outer=(2), stable, constraints=(/2: (/NULL - ]), fd=()-->(2)]
└── eq [type=bool, outer=(2), stable, udf, constraints=(/2: (/NULL - ]), fd=()-->(2)]
├── variable: b:2 [type=int]
└── plus [type=int]
├── udf: fn_immutable [type=int]
Expand Down Expand Up @@ -188,7 +188,7 @@ project
│ ├── prune: (1-4)
│ └── interesting orderings: (+1)
└── filters
└── eq [type=bool, outer=(2), constraints=(/2: (/NULL - ]), fd=()-->(2)]
└── eq [type=bool, outer=(2), udf, constraints=(/2: (/NULL - ]), fd=()-->(2)]
├── variable: b:2 [type=int]
└── udf: fn_leakproof [type=int]
└── body
Expand Down
6 changes: 4 additions & 2 deletions pkg/sql/opt/props/logical.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,12 @@ type Shared struct {

// HasSubquery is true if the subtree rooted at this node contains a subquery.
// The subquery can be a Subquery, Exists, Any, or ArrayFlatten expression.
// Subqueries are the only place where a relational node can be nested within a
// scalar expression.
HasSubquery bool

// HasUDF is true if the subtree rooted at this node contains a UDF
// invocation.
HasUDF bool

// HasCorrelatedSubquery is true if the scalar expression tree contains a
// subquery having one or more outer columns. The subquery can be a Subquery,
// Exists, or Any operator. These operators usually need to be hoisted out of
Expand Down

0 comments on commit 44c714c

Please sign in to comment.