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

Variables should use status var interface #9381

Closed
morgo opened this issue Feb 20, 2019 · 7 comments · Fixed by #20289
Closed

Variables should use status var interface #9381

morgo opened this issue Feb 20, 2019 · 7 comments · Fixed by #20289
Labels
type/enhancement The issue or PR belongs to an enhancement.

Comments

@morgo
Copy link
Contributor

morgo commented Feb 20, 2019

Feature Request

Is your feature request related to a problem? Please describe:

I found a bug in #9365

It looks like the variables are initialized before the config file is parsed. So if the port is non-default, it won't be reflected.

Describe the feature you'd like:

Instead of having configuration all in one place, I think a nicer design would be to have individual components 'register' sysvars. This would make it work closer to how status vars currently work.

The advantage would be that plugins could also add additional variables to the server.

(This would make it work similar to MySQL too..)

Describe alternatives you've considered:

An easy workaround would be to update the sysvars after the server starts.

Teachability, Documentation, Adoption, Migration Strategy:

It would take some initial work to move sysvars to the correct component. Initially, they could all be put under the server.

@morgo
Copy link
Contributor Author

morgo commented Feb 20, 2019

PTAL @tiancaiamao

Do you see any flaws in this strategy?

@tiancaiamao
Copy link
Contributor

individual components 'register' sysvars sounds a good idea.
/cc @lysu

@lysu
Copy link
Contributor

lysu commented Feb 21, 2019

+1 plugin also needs a mechanism to "register sysvar" into hosted TiDB too, and now plugins use ugly way to do it. we should improve it.

@lysu lysu self-assigned this Feb 21, 2019
@morgo morgo added the type/enhancement The issue or PR belongs to an enhancement. label Feb 21, 2019
@ghost
Copy link

ghost commented Sep 24, 2020

@lysu do you mind if I take this over?

@lysu
Copy link
Contributor

lysu commented Sep 25, 2020

@lysu do you mind if I take this over?

:D surely maybe after finish #18717 then maybe have time to do this

@ghost
Copy link

ghost commented Sep 25, 2020

I meant I would like to work on this if that is okay with you :)

I think it works best as multiple PRs:

  1. Convert variables to using status var interface.
  2. Refine the sysVar struct to hold min/max/type/validation (anonymous function), and move it out of ValidateSetSystemVar: https://github.com/pingcap/tidb/blob/master/sessionctx/variable/varsutil.go#L326-L327
  3. Fix Support for typed system variables when running SELECT @@foreign_key_checks #15131
  4. Add an information_schema.variables_info table (based on what is there in MySQL 8.0).
  5. Start work on instance scoped variables. i.e. instance/node level variables #19332

@lysu
Copy link
Contributor

lysu commented Sep 28, 2020

I meant I would like to work on this if that is okay with you :)

I think it works best as multiple PRs:

  1. Convert variables to using status var interface.
  2. Refine the sysVar struct to hold min/max/type/validation (anonymous function), and move it out of ValidateSetSystemVar: https://github.com/pingcap/tidb/blob/master/sessionctx/variable/varsutil.go#L326-L327
  3. Fix Support for typed system variables when running SELECT @@foreign_key_checks #15131
  4. Add an information_schema.variables_info table (based on what is there in MySQL 8.0).
  5. Start work on instance scoped variables. i.e. instance/node level variables #19332

👍 Yes, it's ok for me~ 😊

@lysu lysu removed their assignment Sep 28, 2020
@ghost ghost self-assigned this Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants