Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

planner: Adjust the log level and returned value when cacheableChecker check *ast.TableName nodes #46831

Merged
merged 11 commits into from
Sep 11, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions planner/core/plan_cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ import (
var (
// PlanCacheKeyTestIssue43667 is only for test.
PlanCacheKeyTestIssue43667 struct{}
// PlanCacheKeyTestIssue46760 is only for test.
PlanCacheKeyTestIssue46760 struct{}
)

// SetParameterValuesIntoSCtx sets these parameters into session context.
Expand Down
2 changes: 1 addition & 1 deletion planner/core/plan_cache_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ func GeneratePlanCacheStmtWithAST(ctx context.Context, sctx sessionctx.Context,
reason = "plan cache is disabled"
} else {
if isPrepStmt {
cacheable, reason = CacheableWithCtx(sctx, paramStmt, ret.InfoSchema)
cacheable, reason = IsASTCacheable(ctx, sctx, paramStmt, ret.InfoSchema)
} else {
cacheable = true // it is already checked here
}
Expand Down
76 changes: 56 additions & 20 deletions planner/core/plan_cacheable_checker.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
package core

import (
"context"
"errors"
"fmt"
"math"
"sync"
Expand All @@ -31,20 +33,28 @@ import (
"github.com/pingcap/tidb/types"
driver "github.com/pingcap/tidb/types/parser_driver"
"github.com/pingcap/tidb/util/filter"
"github.com/pingcap/tidb/util/intest"
"github.com/pingcap/tidb/util/logutil"
"go.uber.org/zap"
)

// Cacheable checks whether the input ast(query) is cacheable with empty session context, which is mainly for testing.
// TODO: only for test, remove this function later on.
func Cacheable(node ast.Node, is infoschema.InfoSchema) bool {
c, _ := CacheableWithCtx(nil, node, is)
c, _ := IsASTCacheable(nil, nil, node, is)
return c
}

// CacheableWithCtx checks whether the input ast(query) is cacheable.
// TODO: only for test, remove this function later on.
func CacheableWithCtx(sctx sessionctx.Context, node ast.Node, is infoschema.InfoSchema) (bool, string) {
return IsASTCacheable(nil, sctx, node, is)
}

// IsASTCacheable checks whether the input ast(query) is cacheable.
// Handle "ignore_plan_cache()" hint
// If there are multiple hints, only one will take effect
func CacheableWithCtx(sctx sessionctx.Context, node ast.Node, is infoschema.InfoSchema) (bool, string) {
func IsASTCacheable(ctx context.Context, sctx sessionctx.Context, node ast.Node, is infoschema.InfoSchema) (bool, string) {
_, isSelect := node.(*ast.SelectStmt)
_, isUpdate := node.(*ast.UpdateStmt)
_, isInsert := node.(*ast.InsertStmt)
Expand All @@ -54,6 +64,7 @@ func CacheableWithCtx(sctx sessionctx.Context, node ast.Node, is infoschema.Info
return false, "not a SELECT/UPDATE/INSERT/DELETE/SET statement"
}
checker := cacheableChecker{
ctx: ctx,
sctx: sctx,
cacheable: true,
schema: is,
Expand All @@ -66,6 +77,7 @@ func CacheableWithCtx(sctx sessionctx.Context, node ast.Node, is infoschema.Info

// cacheableChecker checks whether a query can be cached:
type cacheableChecker struct {
ctx context.Context
sctx sessionctx.Context
cacheable bool
schema infoschema.InfoSchema
Expand Down Expand Up @@ -185,7 +197,14 @@ func (checker *cacheableChecker) Enter(in ast.Node) (out ast.Node, skipChildren
}
case *ast.TableName:
if checker.schema != nil {
if isPartitionTable(checker.schema, node) {
isPartitioned, err := isPartitionTable(checker.schema, node)
if err != nil {
checker.cacheable = false
checker.reason = fmt.Errorf("check partition table failed: %w", err).Error()
logutil.BgLogger().Warn("check partition table failed", zap.Error(err))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to add SQL info to this log?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, PTAL

return in, true
}
if isPartitioned {
// Temporary disable prepared plan cache until https://github.com/pingcap/tidb/issues/33031
// is fixed and additional tests with dynamic partition prune mode has been added.
/*
Expand All @@ -197,12 +216,28 @@ func (checker *cacheableChecker) Enter(in ast.Node) (out ast.Node, skipChildren
checker.reason = "query accesses partitioned tables is un-cacheable"
return in, true
}
if hasGeneratedCol(checker.schema, node) {

hasGenCols, err := hasGeneratedCol(checker.schema, node)
if err != nil {
checker.cacheable = false
checker.reason = fmt.Errorf("check generated column failed: %w", err).Error()
logutil.BgLogger().Warn("check generated column failed", zap.Error(err))
return in, true
}
if hasGenCols {
checker.cacheable = false
checker.reason = "query accesses generated columns is un-cacheable"
return in, true
}
if isTempTable(checker.schema, node) {

isTempTbl, err := isTempTable(checker.ctx, checker.schema, node)
if err != nil {
checker.cacheable = false
checker.reason = fmt.Errorf("check temporary table failed: %w", err).Error()
logutil.BgLogger().Warn("check temporary table failed", zap.Error(err))
return in, true
}
if isTempTbl {
checker.cacheable = false
checker.reason = "query accesses temporary tables is un-cacheable"
return in, true
Expand Down Expand Up @@ -554,18 +589,17 @@ func (*nonPreparedPlanCacheableChecker) isFilterNode(node ast.Node) bool {
return false
}

func hasGeneratedCol(schema infoschema.InfoSchema, tn *ast.TableName) bool {
func hasGeneratedCol(schema infoschema.InfoSchema, tn *ast.TableName) (bool, error) {
tb, err := schema.TableByName(tn.Schema, tn.Name)
if err != nil {
logutil.BgLogger().Error("Error occur in checking cacheable", zap.Error(err))
return false
return false, err
}
for _, col := range tb.Cols() {
if col.IsGenerated() {
return true
return true, nil
}
}
return false
return false, nil
}

func getColType(schema infoschema.InfoSchema, tbl *ast.TableName, col *ast.ColumnName) (colType byte, found bool) {
Expand All @@ -584,28 +618,30 @@ func getColType(schema infoschema.InfoSchema, tbl *ast.TableName, col *ast.Colum
return 0, false
}

func isTempTable(schema infoschema.InfoSchema, tn *ast.TableName) bool {
func isTempTable(ctx context.Context, schema infoschema.InfoSchema, tn *ast.TableName) (bool, error) {
if intest.InTest && ctx != nil && ctx.Value(PlanCacheKeyTestIssue46760) != nil {
return false, errors.New("mock error")
}

tb, err := schema.TableByName(tn.Schema, tn.Name)
if err != nil {
logutil.BgLogger().Error("Error occur in checking cacheable", zap.Error(err))
return false
return false, err
}
if tb.Meta().TempTableType != model.TempTableNone {
return true
return true, nil
}
return false
return false, nil
}

func isPartitionTable(schema infoschema.InfoSchema, tn *ast.TableName) bool {
func isPartitionTable(schema infoschema.InfoSchema, tn *ast.TableName) (bool, error) {
tb, err := schema.TableByName(tn.Schema, tn.Name)
if err != nil {
logutil.BgLogger().Error("Error occur in checking cacheable", zap.Error(err))
return false
return false, err
}
if tb.Meta().GetPartitionInfo() != nil {
return true
return true, nil
}
return false
return false, nil
}

// isPlanCacheable returns whether this plan is cacheable and the reason if not.
Expand Down
21 changes: 21 additions & 0 deletions planner/core/plan_cacheable_checker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package core_test

import (
"context"
"fmt"
"strings"
"testing"
Expand Down Expand Up @@ -89,6 +90,26 @@ func TestFixControl44823(t *testing.T) {
tk.MustQuery(`select @@last_plan_from_cache`).Check(testkit.Rows("1"))
}

func TestIssue46760(t *testing.T) {
store := testkit.CreateMockStore(t)
tk := testkit.NewTestKit(t, store)
tk.MustExec("use test")
tk.MustExec(`create table t (a int)`)
tk.MustExec(`prepare st from 'select * from t where a<?'`)
tk.MustExec(`set @a=1`)
tk.MustQuery(`execute st using @a`).Check(testkit.Rows())
tk.MustQuery(`execute st using @a`).Check(testkit.Rows())
tk.MustQuery(`select @@last_plan_from_cache`).Check(testkit.Rows("1"))

ctx := context.WithValue(context.Background(), core.PlanCacheKeyTestIssue46760, struct{}{})
tk.MustExecWithContext(ctx, `prepare st from 'select * from t where a<?'`)
tk.MustQuery(`show warnings`).Check(testkit.Rows("Warning 1105 skip prepared plan-cache: check temporary table failed: mock error"))
tk.MustExec(`set @a=1`)
tk.MustQuery(`execute st using @a`).Check(testkit.Rows())
tk.MustQuery(`execute st using @a`).Check(testkit.Rows())
tk.MustQuery(`select @@last_plan_from_cache`).Check(testkit.Rows("0"))
}

func TestCacheable(t *testing.T) {
store := testkit.CreateMockStore(t)
mockCtx := mock.NewContext()
Expand Down