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

session: add session variable to log query string. #5633

Merged
merged 4 commits into from Jan 16, 2018

Conversation

coocood
Copy link
Member

@coocood coocood commented Jan 13, 2018

Use an atomic global variable to control the switch to log query.
And the query is logged before execution.

Use an atomic global variable to control the switch to log query.
And the query is logged before execution.
@coocood
Copy link
Member Author

coocood commented Jan 13, 2018

@shenli @tiancaiamao PTAL

@@ -620,6 +620,7 @@ var defaultSysVars = []*SysVar{
{ScopeSession, TiDBDMLBatchSize, strconv.Itoa(DefDMLBatchSize)},
{ScopeSession, TiDBCurrentTS, strconv.Itoa(DefCurretTS)},
{ScopeSession, TiDBMaxChunkSize, strconv.Itoa(DefMaxChunkSize)},
{ScopeSession, TiDBLogQuery, strconv.Itoa(DefLogQuery)},
Copy link
Member

Choose a reason for hiding this comment

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

This is a session scope variable. How could the users enable this feature? Most of them do not know it.
I think we could make it a global scope variable or a configuration of the server.

Copy link
Member Author

Choose a reason for hiding this comment

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

This should only be used temporarily and for a single tidb-server.
Make it a configuration require a restart which is inconvenient.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should add a comment here. For this variable is a 'server scope' variable instead of a 'session scope' variable.

@coocood
Copy link
Member Author

coocood commented Jan 14, 2018

@shenli PTAL

@shenli
Copy link
Member

shenli commented Jan 14, 2018

Could we use the same variable with MySQL general log? https://dev.mysql.com/doc/refman/5.7/en/query-log.html

@shenli shenli added this to In progress in production Jan 14, 2018
@coocood
Copy link
Member Author

coocood commented Jan 15, 2018

@shenli
This is a global variable, we don't want to change every tidb-server.

session.go Outdated
}

func logQuery(query string, vars *variable.SessionVars) {
if atomic.LoadUint32(&variable.ProcessLogQuery) != 0 && !vars.InRestrictedSQL {
Copy link
Contributor

Choose a reason for hiding this comment

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

s/0/variable.DefLogQuery

Copy link
Member Author

Choose a reason for hiding this comment

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

The default may change to other values, but 0 is always 0.

@tiancaiamao
Copy link
Contributor

LGTM

@tiancaiamao tiancaiamao added the status/LGT1 Indicates that a PR has LGTM 1. label Jan 15, 2018
Copy link
Contributor

@winkyao winkyao left a comment

Choose a reason for hiding this comment

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

LGTM

@shenli
Copy link
Member

shenli commented Jan 15, 2018

/run-all-tests

@zimulala zimulala merged commit 0961907 into pingcap:master Jan 16, 2018
production automation moved this from In progress to Done Jan 16, 2018
@coocood coocood deleted the log-query branch January 16, 2018 04:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/LGT1 Indicates that a PR has LGTM 1.
Projects
No open projects
production
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants