-
Notifications
You must be signed in to change notification settings - Fork 136
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
feat(stmt): support config maximum number of stmt kept in memory #914
Conversation
name="max_size" | ||
label={t('statement.settings.max_size')} | ||
> | ||
<InputNumber min={1} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a limit on the maximum value?
/cc @breeswish @baurine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice refinement!
err := updateStmtConfig(db, &req) | ||
|
||
if !config.Enable { | ||
err = db.Exec(buildConfigUpdateSQL(&config, "Enable")).Error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this Enable do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extract fields from struct, only the extract fields will be left in SQL.
Like this:
func (t *testConfigSuite) Test_buildConfigUpdateSQL_extract_fields(c *C) {
testConfigStmt := "SET @@GLOBAL.tidb_enable_stmt_summary = true"
c.Assert(buildConfigUpdateSQL(&testConfig{Enable: true, RefreshInterval: 1800}, "Enable"), Equals, testConfigStmt)
}
|
||
val := configValue.Field(i) | ||
column := utils.GetGormColumnName(gormTag) | ||
stmts = append(stmts, fmt.Sprintf("@@GLOBAL.%s = %v", column, val)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems that val
may lead to security risks as it can be anything!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right...Let me think about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can use parameterized queries. It will be very safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, buildConfigQuerySQL
is only used by configHandler
service. And reflect.Value's type is determined. Maybe we can do this refinement after #916 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Co-authored-by: Wenxuan <[email protected]>
Co-authored-by: Wenxuan <[email protected]>
Co-authored-by: Wenxuan <[email protected]>
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by writing |
/merge |
This pull request has been accepted and is ready to merge. Commit hash: 3b3a849
|
* fix(ttlcache): goroutine leak (#892) * tidb: forwarder only uses tidb whose status is Up (#893) * keyviz: Add tips for enabled config (#901) * ui: rocksdb fields (#896) * monitoring: setup sentry (#895) * tidb_client: improve behavior when no alive tidb instance (#900) * feat(stmt): support config maximum number of stmt kept in memory (#914) * feat: debug api (#898) * ui: Improve settings description for Statement (#920) * feat(ui): add tiflash profiling option (#859) * ui: Add a warning for the debug API (#922)
Related: #910