Skip to content

Commit

Permalink
execbuilder: enforce_home_region should only apply to DML
Browse files Browse the repository at this point in the history
Fixes #89875

This fixes a problem where the enforce_home_region session flag might
cause non-DML statements to error out, such as SHOW CREATE, if those
statements utilize scans or joins of multiregion tables.

This also fixes issues with proper erroring out of mutation DML like
UPDATE AND DELETE.

Release note (bug fix): This patch fixes an issue with the
enforce_home_region session setting which may cause SHOW CREATE TABLE or
other non-DML statements to error out if the optimizer plan for the
statement involves accessing a multiregion table.
  • Loading branch information
Mark Sirek committed Oct 14, 2022
1 parent c09bc70 commit 921ae5f
Show file tree
Hide file tree
Showing 23 changed files with 199 additions and 43 deletions.
Expand Up @@ -63,7 +63,6 @@ CREATE TABLE messages_rbr (
account_id INT NOT NULL,
message_id UUID DEFAULT gen_random_uuid(),
message STRING NOT NULL,
crdb_region crdb_internal_region NOT NULL,
PRIMARY KEY (account_id),
INDEX msg_idx(message)
)
Expand Down Expand Up @@ -176,6 +175,83 @@ CREATE TABLE json_arr2_rbt (
statement ok
SET enforce_home_region = true

### Regression tests for issue #89875

# Non-DML statements should not error out due to enforce_home_region.
query T
SELECT table_name FROM [SHOW CREATE messages_global]
----
messages_global

# Non-DML SHOW RANGES statement on RBR table should succeed.
query I
SELECT DISTINCT range_id FROM [SHOW RANGES FROM TABLE messages_rbr]
----
52

# Update does not fail when accessing all rows in messages_rbr because lookup
# join does not error out the lookup table in phase 1.
statement ok
UPDATE messages_rbt SET account_id = -account_id WHERE account_id NOT IN (SELECT account_id FROM messages_rbr)

# Update should fail accessing all rows in messages_rbr.
statement error pq: Query has no home region\. Try adding a filter on messages_rbr\.crdb_region and/or on key column \(messages_rbr\.account_id\)\.
UPDATE messages_rbt SET account_id = -account_id WHERE message_id NOT IN (SELECT message_id FROM messages_rbr)

# Update should fail accessing all rows in messages_rbr.
statement error pq: Query has no home region\. Try adding a filter on messages_rbr\.crdb_region and/or on key column \(messages_rbr\.account_id\)\.
UPDATE messages_rbr SET account_id = -account_id WHERE account_id NOT IN (SELECT account_id FROM messages_rbt)

# Delete does not fail when accessing all rows in messages_rbr because lookup
# join does not error out the lookup table in phase 1.
statement ok
DELETE FROM messages_rbt WHERE account_id NOT IN (SELECT account_id FROM messages_rbr)

# Delete should fail accessing all rows in messages_rbr.
# join does not error out the lookup table in phase 1.
statement error pq: Query has no home region\. Try adding a filter on messages_rbr\.crdb_region and/or on key column \(messages_rbr\.account_id\)\.
DELETE FROM messages_rbt WHERE message_id NOT IN (SELECT message_id FROM messages_rbr)

# Delete of potentially all rows in messages_rbr should fail.
statement error pq: Query has no home region\. Try adding a filter on messages_rbr\.crdb_region and/or on key column \(messages_rbr\.account_id\)\.
DELETE FROM messages_rbr WHERE account_id NOT IN (SELECT account_id FROM messages_rbt)

# Delete accessing all regions should fail.
statement error pq: Query has no home region\. Try adding a filter on messages_rbr\.crdb_region and/or on key column \(messages_rbr\.account_id\)\.
DELETE FROM messages_rbr WHERE message = 'Hello World!'

# Insert should fail accessing all rows in messages_rbr.
statement error pq: Query has no home region\. Try adding a filter on messages_rbr\.crdb_region and/or on key column \(messages_rbr\.account_id\)\.
INSERT INTO messages_rbt SELECT * FROM messages_rbr

# Insert into an RBR table should succeed. New rows are placed in the gateway region.
statement ok
INSERT INTO messages_rbr SELECT * FROM messages_rbt

# Upsert into an RBR table should succeed.
statement ok
UPSERT INTO messages_rbr SELECT * FROM messages_rbt

# Upsert should fail accessing all rows in messages_rbr.
statement error pq: Query has no home region\. Try adding a LIMIT clause\.
UPSERT INTO messages_rbt SELECT * FROM messages_rbr

# Upsert into an RBR table uses locality-optimized lookup join and should
# succeed.
statement ok
UPSERT INTO messages_rbr SELECT * FROM messages_rbt

# UNION ALL where one branch scans all rows of an RBR table should fail.
statement error pq: Query has no home region\. Try adding a LIMIT clause\.
SELECT * FROM messages_rbr UNION ALL SELECT * FROM messages_rbt

# UNION ALL where one branch scans 1 row of an RBR table should succeed.
query TTT
SELECT * FROM (SELECT * FROM messages_rbr LIMIT 1) UNION ALL SELECT * FROM messages_rbt
----

### End regression tests for issue #89875

# A join relation with no home region as the left input of lookup join should
# not be allowed.
statement error pq: Query has no home region\. Try adding a filter on rbr\.crdb_region and/or on key column \(rbr\.account_id\)\.
Expand Down Expand Up @@ -296,7 +372,7 @@ SELECT * FROM customers c JOIN orders o ON c.id = o.cust_id AND
(c.crdb_region = o.crdb_region) WHERE c.id = '69a1c2c2-5b18-459e-94d2-079dc53a4dd0'

# Locality optimized lookup join is allowed.
query TTTTTTT retry
query TTTTTT retry
SELECT * FROM messages_rbr rbr, messages_rbt rbt WHERE rbr.account_id = rbt.account_id LIMIT 1
----

Expand Down Expand Up @@ -331,7 +407,7 @@ SELECT message from messages_rbt@messages_rbt_pkey
----

# A local join between an RBR and RBT table should be allowed.
query TTTTTTT retry
query TTTTTT retry
SELECT * FROM messages_rbt rbt INNER LOOKUP JOIN messages_rbr rbr ON rbr.account_id = rbt.account_id
AND rbr.crdb_region = 'ap-southeast-2'
----
Expand All @@ -340,17 +416,18 @@ query T retry
EXPLAIN(OPT) SELECT * FROM messages_rbt rbt INNER LOOKUP JOIN messages_rbr rbr ON rbr.account_id = rbt.account_id
AND rbr.crdb_region = 'ap-southeast-2'
----
inner-join (lookup messages_rbr [as=rbr])
├── flags: force lookup join (into right side)
├── lookup columns are key
├── project
│ ├── scan messages_rbt [as=rbt]
│ └── projections
│ └── 'ap-southeast-2'
└── filters (true)
project
└── inner-join (lookup messages_rbr [as=rbr])
├── flags: force lookup join (into right side)
├── lookup columns are key
├── project
│ ├── scan messages_rbt [as=rbt]
│ └── projections
│ └── 'ap-southeast-2'
└── filters (true)

# A local join between an RBR and RBT table should be allowed.
query TTTTTTT retry
query TTTTTT retry
SELECT * FROM messages_rbr rbr INNER LOOKUP JOIN messages_rbt rbt ON rbr.account_id = rbt.account_id
AND rbr.crdb_region = 'ap-southeast-2'
----
Expand All @@ -359,12 +436,13 @@ query T retry
EXPLAIN(OPT) SELECT * FROM messages_rbr rbr INNER LOOKUP JOIN messages_rbt rbt ON rbr.account_id = rbt.account_id
AND rbr.crdb_region = 'ap-southeast-2'
----
inner-join (lookup messages_rbt [as=rbt])
├── flags: force lookup join (into right side)
├── lookup columns are key
├── scan messages_rbr [as=rbr]
│ └── constraint: /4/1: [/'ap-southeast-2' - /'ap-southeast-2']
└── filters (true)
project
└── inner-join (lookup messages_rbt [as=rbt])
├── flags: force lookup join (into right side)
├── lookup columns are key
├── scan messages_rbr [as=rbr]
│ └── constraint: /4/1: [/'ap-southeast-2' - /'ap-southeast-2']
└── filters (true)

# A lookup join with a global table as either input should be allowed.
query TTTTTT retry
Expand Down Expand Up @@ -414,7 +492,7 @@ SELECT * FROM messages_rbr_alt rbr INNER LOOKUP JOIN messages_global g2 ON rbr.a

# A lookup join relation with a left input join relation which uses locality
# optimized scan in one of the tables of the lookup join should be allowed.
query TTTTTTTTTT retry
query TTTTTTTTT retry
SELECT * FROM (SELECT * FROM messages_rbr LIMIT 1) rbr INNER LOOKUP JOIN
messages_global g2 ON rbr.account_id = g2.account_id
INNER LOOKUP JOIN messages_global g3 ON g2.account_id = g3.account_id
Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/constraint.go
Expand Up @@ -155,7 +155,7 @@ func (p *planner) ConstrainPrimaryIndexSpanByExpr(
remainingFilter = tree.DBoolTrue
} else {
eb := execbuilder.New(ctx, newExecFactory(ctx, p), &p.optPlanningCtx.optimizer,
nf.Memo(), &oc, &remaining, evalCtx, false)
nf.Memo(), &oc, &remaining, evalCtx, false, p.IsANSIDML())
eb.SetBuiltinFuncWrapper(semaCtx.FunctionResolver)
expr, err := eb.BuildScalar()
if err != nil {
Expand Down
5 changes: 5 additions & 0 deletions pkg/sql/faketreeeval/evalctx.go
Expand Up @@ -464,6 +464,11 @@ func (ep *DummyEvalPlanner) GetMultiregionConfig(
return nil /* regionConfig */, false
}

// IsANSIDML is part of the eval.Planner interface.
func (ep *DummyEvalPlanner) IsANSIDML() bool {
return false
}

// DummyPrivilegedAccessor implements the tree.PrivilegedAccessor interface by returning errors.
type DummyPrivilegedAccessor struct{}

Expand Down
4 changes: 3 additions & 1 deletion pkg/sql/opt/bench/bench_test.go
Expand Up @@ -713,6 +713,7 @@ func (h *harness) runSimple(tb testing.TB, query benchQuery, phase Phase) {
root,
&h.evalCtx,
true, /* allowAutoCommit */
stmt.IsANSIDML(),
)
if _, err = eb.Build(); err != nil {
tb.Fatalf("%v", err)
Expand Down Expand Up @@ -766,7 +767,8 @@ func (h *harness) runPrepared(tb testing.TB, phase Phase) {
nil, /* catalog */
root,
&h.evalCtx,
true, /* allowAutoCommit */
true, /* allowAutoCommit */
false, /* isANSIDML */
)
if _, err := eb.Build(); err != nil {
tb.Fatalf("%v", err)
Expand Down
7 changes: 7 additions & 0 deletions pkg/sql/opt/exec/execbuilder/builder.go
Expand Up @@ -181,6 +181,11 @@ type Builder struct {
// doScanExprCollection, when true, causes buildScan to add any ScanExprs it
// processes to the builtScans slice.
doScanExprCollection bool

// IsANSIDML is true if the AST the execbuilder is working on is one of the
// 4 DML statements, SELECT, UPDATE, INSERT, DELETE, or an EXPLAIN of one of
// these statements.
IsANSIDML bool
}

// New constructs an instance of the execution node builder using the
Expand All @@ -202,6 +207,7 @@ func New(
e opt.Expr,
evalCtx *eval.Context,
allowAutoCommit bool,
isANSIDML bool,
) *Builder {
b := &Builder{
factory: factory,
Expand All @@ -213,6 +219,7 @@ func New(
evalCtx: evalCtx,
allowAutoCommit: allowAutoCommit,
initialAllowAutoCommit: allowAutoCommit,
IsANSIDML: isANSIDML,
}
if evalCtx != nil {
sd := evalCtx.SessionData()
Expand Down
3 changes: 2 additions & 1 deletion pkg/sql/opt/exec/execbuilder/cascades.go
Expand Up @@ -293,7 +293,8 @@ func (cb *cascadeBuilder) planCascade(
}

// 5. Execbuild the optimized expression.
eb := New(ctx, execFactory, &o, factory.Memo(), cb.b.catalog, optimizedExpr, evalCtx, allowAutoCommit)
eb := New(ctx, execFactory, &o, factory.Memo(), cb.b.catalog, optimizedExpr, evalCtx, allowAutoCommit,
evalCtx.Planner.IsANSIDML())
if bufferRef != nil {
// Set up the With binding.
eb.addBuiltWithExpr(cascadeInputWithID, bufferColMap, bufferRef)
Expand Down
1 change: 1 addition & 0 deletions pkg/sql/opt/exec/execbuilder/format.go
Expand Up @@ -49,6 +49,7 @@ func fmtInterceptor(f *memo.ExprFmtCtx, scalar opt.ScalarExpr) string {
scalar,
nil, /* evalCtx */
false, /* allowAutoCommit */
false, /* isANSIDML */
)
expr, err := bld.BuildScalar()
if err != nil {
Expand Down
46 changes: 34 additions & 12 deletions pkg/sql/opt/exec/execbuilder/relational.go
Expand Up @@ -812,7 +812,7 @@ func (b *Builder) buildScan(scan *memo.ScanExpr) (execPlan, error) {
}

res.root = root
if b.evalCtx.SessionData().EnforceHomeRegion && b.doScanExprCollection {
if b.evalCtx.SessionData().EnforceHomeRegion && b.IsANSIDML && b.doScanExprCollection {
if b.builtScans == nil {
// Make this large enough to handle simple 2-table join queries without
// wasting memory.
Expand Down Expand Up @@ -1137,7 +1137,7 @@ func (b *Builder) buildApplyJoin(join memo.RelExpr) (execPlan, error) {
return nil, err
}

eb := New(ctx, ef, &o, f.Memo(), b.catalog, newRightSide, b.evalCtx, false /* allowAutoCommit */)
eb := New(ctx, ef, &o, f.Memo(), b.catalog, newRightSide, b.evalCtx, false /* allowAutoCommit */, b.IsANSIDML)
eb.disableTelemetry = true
eb.withExprs = withExprs
plan, err := eb.Build()
Expand Down Expand Up @@ -1829,7 +1829,7 @@ func (b *Builder) buildSort(sort *memo.SortExpr) (execPlan, error) {
return execPlan{root: node, outputCols: input.outputCols}, nil
}

func (b *Builder) enforceScanWithHomeRegion() error {
func (b *Builder) enforceScanWithHomeRegion(skipID cat.StableID) error {
homeRegion := ""
firstTable := ""
gatewayRegion, foundLocalRegion := b.evalCtx.Locality.Find("region")
Expand All @@ -1839,6 +1839,9 @@ func (b *Builder) enforceScanWithHomeRegion() error {
for i, scan := range b.builtScans {
inputTableMeta := scan.Memo().Metadata().TableMeta(scan.Table)
inputTable := inputTableMeta.Table
if inputTable.ID() == skipID {
continue
}
inputTableName := string(inputTable.Name())
inputIndexOrdinal := scan.Index

Expand Down Expand Up @@ -1890,7 +1893,20 @@ func (b *Builder) buildDistribute(distribute *memo.DistributeExpr) (input execPl
return input, err
}

if b.evalCtx.SessionData().EnforceHomeRegion {
if b.evalCtx.SessionData().EnforceHomeRegion && b.IsANSIDML {
var mutationTableID opt.TableID
if updateExpr, ok := distribute.Input.(*memo.UpdateExpr); ok {
mutationTableID = updateExpr.Table
} else if deleteExpr, ok := distribute.Input.(*memo.DeleteExpr); ok {
mutationTableID = deleteExpr.Table
}
var mutationTabMeta *opt.TableMeta
var mutationStableID cat.StableID
if mutationTableID != opt.TableID(0) {
md := b.mem.Metadata()
mutationTabMeta = md.TableMeta(mutationTableID)
mutationStableID = mutationTabMeta.Table.ID()
}
saveDoScanExprCollection := b.doScanExprCollection
b.doScanExprCollection = true
// Traverse the tree again, this time collecting ScanExprs that should
Expand All @@ -1900,7 +1916,7 @@ func (b *Builder) buildDistribute(distribute *memo.DistributeExpr) (input execPl
if err != nil {
return execPlan{}, err
}
err = b.enforceScanWithHomeRegion()
err = b.enforceScanWithHomeRegion(mutationStableID)
if err != nil {
return execPlan{}, err
}
Expand All @@ -1915,12 +1931,18 @@ func (b *Builder) buildDistribute(distribute *memo.DistributeExpr) (input execPl
} else if distribute.Input.Op() != opt.LookupJoinOp {
// More detailed error message handling for lookup join occurs in the
// execbuilder.
errCode = pgcode.QueryHasNoHomeRegion
errorStringBuilder.WriteString("Query has no home region.")
errorStringBuilder.WriteString(` Try adding a LIMIT clause.`)
if mutationTableID != opt.TableID(0) {
err = b.filterSuggestionError(mutationTabMeta, 0 /* indexOrdinal1 */, nil /* table2Meta */, 0 /* indexOrdinal2 */)
} else {
errCode = pgcode.QueryHasNoHomeRegion
errorStringBuilder.WriteString("Query has no home region.")
errorStringBuilder.WriteString(` Try adding a LIMIT clause.`)
}
}
if err == nil {
msgString := errorStringBuilder.String()
err = pgerror.Newf(errCode, "%s", msgString)
}
msgString := errorStringBuilder.String()
err = pgerror.Newf(errCode, "%s", msgString)
return execPlan{}, err
}

Expand Down Expand Up @@ -2171,7 +2193,7 @@ func (b *Builder) buildLookupJoin(join *memo.LookupJoinExpr) (execPlan, error) {
}
}
saveDoScanExprCollection := false
enforceHomeRegion := b.evalCtx.SessionData().EnforceHomeRegion
enforceHomeRegion := b.evalCtx.SessionData().EnforceHomeRegion && b.IsANSIDML
if enforceHomeRegion {
saveDoScanExprCollection = b.doScanExprCollection
var rel opt.Expr
Expand Down Expand Up @@ -2389,7 +2411,7 @@ func (b *Builder) handleRemoteInvertedJoinError(join *memo.InvertedJoinExpr) (er
}

func (b *Builder) buildInvertedJoin(join *memo.InvertedJoinExpr) (execPlan, error) {
enforceHomeRegion := b.evalCtx.SessionData().EnforceHomeRegion
enforceHomeRegion := b.evalCtx.SessionData().EnforceHomeRegion && b.IsANSIDML
saveDoScanExprCollection := false
if enforceHomeRegion {
saveDoScanExprCollection = b.doScanExprCollection
Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/opt/exec/execbuilder/scalar.go
Expand Up @@ -721,7 +721,7 @@ func (b *Builder) buildUDF(ctx *buildScalarCtx, scalar opt.ScalarExpr) (tree.Typ
// TODO(mgartner): Add support for WITH expressions inside UDF bodies.
// TODO(mgartner): Add support for subqueries inside UDF bodies.
ef := ref.(exec.Factory)
eb := New(ctx, ef, &o, f.Memo(), b.catalog, newRightSide, b.evalCtx, false /* allowAutoCommit */)
eb := New(ctx, ef, &o, f.Memo(), b.catalog, newRightSide, b.evalCtx, false /* allowAutoCommit */, b.IsANSIDML)
eb.disableTelemetry = true
plan, err := eb.Build()
if err != nil {
Expand Down
1 change: 1 addition & 0 deletions pkg/sql/opt/exec/execbuilder/statement.go
Expand Up @@ -152,6 +152,7 @@ func (b *Builder) buildExplain(explainExpr *memo.ExplainExpr) (execPlan, error)

explainBld := New(
b.ctx, ef, b.optimizer, b.mem, b.catalog, explainExpr.Input, b.evalCtx, b.initialAllowAutoCommit,
b.IsANSIDML,
)
explainBld.disableTelemetry = true
plan, err := explainBld.Build()
Expand Down
1 change: 1 addition & 0 deletions pkg/sql/opt/idxconstraint/index_constraints_test.go
Expand Up @@ -138,6 +138,7 @@ func TestIndexConstraints(t *testing.T) {
execBld := execbuilder.New(
context.Background(), nil /* execFactory */, nil /* optimizer */, f.Memo(), nil, /* catalog */
&remainingFilter, &evalCtx, false, /* allowAutoCommit */
false, /* isANSIDML */
)
expr, err := execBld.BuildScalar()
if err != nil {
Expand Down
1 change: 1 addition & 0 deletions pkg/sql/opt/lookupjoin/constraint_builder_test.go
Expand Up @@ -321,6 +321,7 @@ func formatScalar(e opt.Expr, f *norm.Factory, evalCtx *eval.Context) string {
execBld := execbuilder.New(
context.Background(), nil /* execFactory */, nil /* optimizer */, f.Memo(), nil, /* catalog */
e, evalCtx, false, /* allowAutoCommit */
false, /* isANSIDML */
)
expr, err := execBld.BuildScalar()
if err != nil {
Expand Down

0 comments on commit 921ae5f

Please sign in to comment.