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.

Sign up for GitHub

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

sessionctx: set skipInit false for TiDBOptProjectionPushDown and TiDBOptAggPushDown #35491

Merged
merged 13 commits into from
Jun 21, 2022
+23 −2
Merged

sessionctx: set skipInit false for TiDBOptProjectionPushDown and TiDBOptAggPushDown #35491

Changes from 2 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
81cabd0
set skipInit to false
xuyifangreeneyes Jun 18, 2022
617671c
Update sessionctx/variable/sysvar.go
xuyifangreeneyes Jun 19, 2022
682fa12
add ut and revert for TiDBOptAggPushDown
xuyifangreeneyes Jun 19, 2022
f42d5f2
remove SkipInit for TiDBOptAggPushDown
xuyifangreeneyes Jun 20, 2022
9bb15b1
Merge branch 'master' into skip-init
ti-chi-bot Jun 20, 2022
b5b225c
Merge branch 'master' into skip-init
ti-chi-bot Jun 20, 2022
2be50e7
Merge branch 'master' into skip-init
hawkingrei Jun 20, 2022
14245f3
Merge branch 'master' into skip-init
hawkingrei Jun 20, 2022
a98ad63
Merge branch 'master' into skip-init
ti-chi-bot Jun 20, 2022
ba66e06
Merge branch 'master' into skip-init
ti-chi-bot Jun 21, 2022
d58c8cb
Merge branch 'master' into skip-init
ti-chi-bot Jun 21, 2022
bdc5fdc
Merge branch 'master' into skip-init
ti-chi-bot Jun 21, 2022
1d4d7fa
Merge branch 'master' into skip-init
ti-chi-bot Jun 21, 2022
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
4 changes: 2 additions & 2 deletions sessionctx/variable/sysvar.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,11 +140,11 @@ var defaultSysVars = []*SysVar{
}
return nil
}},
{Scope: ScopeSession, Name: TiDBOptProjectionPushDown, Value: BoolToOnOff(config.GetGlobalConfig().Performance.ProjectionPushDown), skipInit: true, Type: TypeBool, SetSession: func(s *SessionVars, val string) error {
{Scope: ScopeSession, Name: TiDBOptProjectionPushDown, Value: BoolToOnOff(config.GetGlobalConfig().Performance.ProjectionPushDown), Type: TypeBool, SetSession: func(s *SessionVars, val string) error {
s.AllowProjectionPushDown = TiDBOptOn(val)
return nil
}},
{Scope: ScopeSession, Name: TiDBOptAggPushDown, Value: BoolToOnOff(DefOptAggPushDown), Type: TypeBool, skipInit: true, SetSession: func(s *SessionVars, val string) error {
{Scope: ScopeSession, Name: TiDBOptAggPushDown, Value: BoolToOnOff(DefOptAggPushDown), Type: TypeBool, skipInit: false, SetSession: func(s *SessionVars, val string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same with this one

Suggested change
{Scope: ScopeSession, Name: TiDBOptAggPushDown, Value: BoolToOnOff(DefOptAggPushDown), Type: TypeBool, skipInit: false, SetSession: func(s *SessionVars, val string) error {
{Scope: ScopeSession, Name: TiDBOptAggPushDown, Value: BoolToOnOff(DefOptAggPushDown), Type: TypeBool, SetSession: func(s *SessionVars, val string) error {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since DefOptAggPushDown is false, TiDBOptAggPushDown doesn't have the problem like #35083 even if skipInit is true. Maybe we can just keep it?

Copy link
Contributor

Choose a reason for hiding this comment

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

The only known required use cases for skipInit are:

  1. old-style INSTANCE scoped variables (which use SESSION). There are only 2 of these left: TiDBLogFileMaxDays and TiDBConfig should be ScopeInstance #35190
  2. when you set the charset, it automatically sets the default collation for that charset.

We can use a workaround to check if it is a user issuing a Set statement for (2), so eventually we should be able to remove skipInit completely. The problem is because it exists, everyone keeps using it incorrectly (adding new cases that skipInit).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

skipInit of TiDBOptAggPushDown is removed now.

s.AllowAggPushDown = TiDBOptOn(val)
return nil
}},
Expand Down