Skip to content

Commit

Permalink
sql: require placeholder types to be identical to use a cached plan
Browse files Browse the repository at this point in the history
Release note (bug fix): Fix a bug where a prepared statement could
incorrectly reuse the query plan of a different prepared statements that
had similar, but not identical type hints.
  • Loading branch information
rafiss committed Jul 15, 2021
1 parent aa95069 commit 62030be
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 11 deletions.
2 changes: 1 addition & 1 deletion pkg/sql/plan_opt.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ func (p *planner) prepareUsingOptimizer(ctx context.Context) (planFlags, error)
if ok && cachedData.PrepareMetadata != nil {
pm := cachedData.PrepareMetadata
// Check that the type hints match (the type hints affect type checking).
if !pm.TypeHints.Equals(p.semaCtx.Placeholders.TypeHints) {
if !pm.TypeHints.Identical(p.semaCtx.Placeholders.TypeHints) {
opc.log(ctx, "query cache hit but type hints don't match")
} else {
isStale, err := cachedData.Memo.IsStale(ctx, p.EvalContext(), &opc.catalog)
Expand Down
6 changes: 3 additions & 3 deletions pkg/sql/sem/tree/placeholders.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@ func (idx PlaceholderIdx) String() string {
// placeholders in the statement. Entries that don't yet have a type are nil.
type PlaceholderTypes []*types.T

// Equals returns true if two PlaceholderTypes contain the same types.
func (pt PlaceholderTypes) Equals(other PlaceholderTypes) bool {
// Identical returns true if two PlaceholderTypes contain the same types.
func (pt PlaceholderTypes) Identical(other PlaceholderTypes) bool {
if len(pt) != len(other) {
return false
}
Expand All @@ -50,7 +50,7 @@ func (pt PlaceholderTypes) Equals(other PlaceholderTypes) bool {
case t == nil && other[i] == nil:
case t == nil || other[i] == nil:
return false
case !t.Equivalent(other[i]):
case !t.Identical(other[i]):
return false
}
}
Expand Down
29 changes: 22 additions & 7 deletions pkg/sql/sem/tree/placeholders_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,12 @@ import (
"github.com/cockroachdb/cockroach/pkg/util/log"
)

func TestPlaceholderTypesEquals(t *testing.T) {
func TestPlaceholderTypesIdentical(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)
testCases := []struct {
a, b PlaceholderTypes
equal bool
a, b PlaceholderTypes
identical bool
}{
{ // 0
PlaceholderTypes{},
Expand Down Expand Up @@ -61,14 +61,29 @@ func TestPlaceholderTypesEquals(t *testing.T) {
PlaceholderTypes{types.Int, nil},
false,
},
{ // 7
PlaceholderTypes{types.Int, nil},
PlaceholderTypes{types.Int4, nil},
false,
},
{ // 8
PlaceholderTypes{types.Int},
PlaceholderTypes{types.Int4},
false,
},
{ // 9
PlaceholderTypes{types.Int4},
PlaceholderTypes{types.Int4},
true,
},
}
for i, tc := range testCases {
t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
res := tc.a.Equals(tc.b)
if res != tc.equal {
t.Errorf("%v vs %v: expected %t, got %t", tc.a, tc.b, tc.equal, res)
res := tc.a.Identical(tc.b)
if res != tc.identical {
t.Errorf("%v vs %v: expected %t, got %t", tc.a, tc.b, tc.identical, res)
}
res2 := tc.b.Equals(tc.a)
res2 := tc.b.Identical(tc.a)
if res != res2 {
t.Errorf("%v vs %v: not commutative", tc.a, tc.b)
}
Expand Down

0 comments on commit 62030be

Please sign in to comment.