Skip to content

Commit

Permalink
Disallow GET_LOCK() calls in vttablet.
Browse files Browse the repository at this point in the history
Named locks are unsafe with server-side connection pooling.

See #3631 for background.

Signed-off-by: David Weitzman <dweitzman@pinterest.com>
  • Loading branch information
dweitzman committed May 31, 2018
1 parent acc9936 commit 771863d
Show file tree
Hide file tree
Showing 3 changed files with 36 additions and 0 deletions.
4 changes: 4 additions & 0 deletions data/test/tabletserver/exec_cases.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2151,3 +2151,7 @@ options:PassthroughDMLs
# syntax error
"syntax error"
"syntax error at position 7 near 'syntax'"

# named locks are unsafe with server-side connection pooling
"select get_lock('foo') from dual"
"get_lock() not allowed"
4 changes: 4 additions & 0 deletions data/test/tabletserver/stream_cases.txt
Original file line number Diff line number Diff line change
Expand Up @@ -52,3 +52,7 @@
# syntax error
"syntax error"
"syntax error at position 7 near 'syntax'"

# named locks are unsafe with server-side connection pooling
"select get_lock('foo') from dual"
"get_lock() not allowed"
28 changes: 28 additions & 0 deletions go/vt/vttablet/tabletserver/planbuilder/plan.go
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,12 @@ func (plan *Plan) setTable(tableName sqlparser.TableIdent, tables map[string]*sc
func Build(statement sqlparser.Statement, tables map[string]*schema.Table) (*Plan, error) {
var plan *Plan
var err error

err = checkForPoolingUnsafeConstructs(statement)
if err != nil {
return nil, err
}

switch stmt := statement.(type) {
case *sqlparser.Union:
plan, err = &Plan{
Expand Down Expand Up @@ -309,6 +315,11 @@ func BuildStreaming(sql string, tables map[string]*schema.Table) (*Plan, error)
return nil, err
}

err = checkForPoolingUnsafeConstructs(statement)
if err != nil {
return nil, err
}

plan := &Plan{
PlanID: PlanSelectStream,
FullQuery: GenerateFullQuery(statement),
Expand Down Expand Up @@ -350,3 +361,20 @@ func BuildMessageStreaming(name string, tables map[string]*schema.Table) (*Plan,
}}
return plan, nil
}

// checkForPoolingUnsafeConstructs returns an error if the SQL expression contains
// a call to GET_LOCK(), which is unsafe with server-side connection pooling.
// For more background, see https://github.com/vitessio/vitess/issues/3631.
func checkForPoolingUnsafeConstructs(expr sqlparser.SQLNode) error {
return sqlparser.Walk(func(node sqlparser.SQLNode) (kontinue bool, err error) {
if f, ok := node.(*sqlparser.FuncExpr); ok {
if f.Name.Lowered() == "get_lock" {
return false, vterrors.New(vtrpcpb.Code_FAILED_PRECONDITION, "get_lock() not allowed")
}
}

// TODO: This could be smarter about not walking down parts of the AST that can't contain
// function calls.
return true, nil
}, expr)
}

0 comments on commit 771863d

Please sign in to comment.