-
-
Notifications
You must be signed in to change notification settings - Fork 205
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
faster status updates #2615
faster status updates #2615
Conversation
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.
LGTM!
…-server into max/sync-status-updates
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.
Kind of crazy how much perf this unlocks, good find
sql/core.go
Outdated
@@ -713,7 +713,7 @@ type StatusVariableRegistry interface { | |||
// SetGlobal sets the global value of the status variable with the given name, returns an error if the variable is SessionOnly scope | |||
SetGlobal(name string, val interface{}) error | |||
// IncrementGlobal increments the value of the status variable by the given integer value |
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.
Should document failure behavior, kind of important
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.
added note about session-only nooping
v, ok := g.varVals[name] | ||
if !ok || v.Variable().GetScope() == sql.StatusVariableScope_Session { | ||
return sql.ErrUnknownSystemVariable.New(name) | ||
return |
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.
Appropriate to panic here? Basically always means a bug right?
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.
The globals list excludes sessionOnly
*, so it's only ~297/678 of the total list. So we want to ideally not call this on session only, but noop if we 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.
The query type counters have both scopes unfortunately, so I had to leave those for now
System variables can be session, global, or both.
sql.IncrementStatusVariable
is a helper method that primarily helps the "both" category increment the global and session counters for certain variables.Threads_running
is a global only variable that is incremented/decremented every begin/end query, and gets a lot of traffic. The old code usedsql.IncrementStatusVariable
to incrementThreads_running
, which was a particularly expensive way to increment a global var because (1) we'd make a new error for every call to the session updater, and (2) the extra map lookup is unnecessary. We don't do the extra map lookup now, and we weren't using the error return so I removed the return variable.Note: this also refactors status variables to be explicitly initializated in the engine
bump/perf here: dolthub/dolt#8189