stubbing out status variables #2414
Conversation
| @@ -2906,3 +2906,5 @@ var systemVars = map[string]sql.SystemVariable{ | |||
| Default: int8(1), | |||
| }, | |||
| } | |||
|
|
|||
| // TODO: need to implement SystemDateTime type | |||
There was a problem hiding this comment.
(minor) not sure if you meant to leave this in or not
| sess.SetStatusVariable(ctx, "Aborted_clients", int64(100)) | ||
|
|
||
| sessVal, err = sess.GetStatusVariable(ctx, "Aborted_clients") | ||
| require.NoError(err) | ||
| require.Equal(int64(100), sessVal) | ||
|
|
||
| _, globalVal, ok = sql.StatusVariables.GetGlobal("Aborted_clients") | ||
| require.True(ok) | ||
| require.Equal(int64(0), globalVal) |
There was a problem hiding this comment.
It's worth thinking a little more about the possible scopes for status variables: global, session, or both.
For example, Aborted_clients has global scope, so if we set it through the session, I'd expect the next call to GetGlobal to also return the newly set value.
It probably makes more sense to only allow Getting/Setting status variables through the session when their scope is session or both. Then when you access through the session, it's clear that you're getting/setting a status var at a session scope, and when you access through the global API, it's clear that you're getting/setting at a global scope.
| var _ sql.StatusVariableRegistry = (*globalStatusVariables)(nil) | ||
|
|
||
| // NewSessionMap implements sql.StatusVariableRegistry | ||
| func (g *globalStatusVariables) NewSessionMap() map[string]sql.StatusVarValue { |
There was a problem hiding this comment.
Related to the comment in status_variable_test.go about keeping session and global vars accessed separately... it seems like NewSessionMap should probably only copy over status variables that have a session scope (or have both session and global scope). That seems like it'll be a cleaner calling pattern if global status vars are always accessed through a global interface and session status vars are always accessed through the session, plus it means we have to copy over less status vars for each session, which is nice.
| StatusVariableScope_Both | ||
| ) | ||
|
|
||
| type StatusVariable interface { |
There was a problem hiding this comment.
(minor) Even though this interface is super simple, it's still worth taking a couple of minutes to add some doc strings. If it's important enough to be an interface, it's generally worthwhile to add a sentence or two for the interface and methods.
(Alternatively, you could probably get rid of this interface and just expose the struct with this name. I see you matched the new interface for system vars that was added to give Doltgres more flexibility, but I don't think Postgres has the same concept of "status variables" like MySQL does, so it's probably not strictly needed. I don't have a strong opinion either way, and would probably just leave it in like you have it, just to match SystemVariables, but figured I'd mention it.)
This PR adds the initial implementation of Status Variables.
There are 682 status variables, and are very similar to System Variables.
Every variable is read-only (and can only be updated by the server itself), and there are session-specific variables.
MySQL Docs: https://dev.mysql.com/doc/refman/8.0/en/server-status-variable-reference.html
Related: dolthub/dolt#7646