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

faster status updates #2615

Merged
merged 9 commits into from
Aug 1, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions engine.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import (
"github.com/dolthub/go-mysql-server/sql/rowexec"
"github.com/dolthub/go-mysql-server/sql/transform"
"github.com/dolthub/go-mysql-server/sql/types"
"github.com/dolthub/go-mysql-server/sql/variables"
_ "github.com/dolthub/go-mysql-server/sql/variables"
)

Expand Down Expand Up @@ -168,6 +169,8 @@ func New(a *analyzer.Analyzer, cfg *Config) *Engine {

ls := sql.NewLockSubsystem()

variables.InitStatusVariables()

emptyCtx := sql.NewEmptyContext()

if fn, err := a.Catalog.Function(emptyCtx, "version"); fn == nil || err != nil {
Expand Down
6 changes: 3 additions & 3 deletions processlist.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,9 @@ func (pl *ProcessList) Processes() []sql.Process {
}

func (pl *ProcessList) AddConnection(id uint32, addr string) {
sql.StatusVariables.IncrementGlobal("Threads_connected", 1)
pl.mu.Lock()
defer pl.mu.Unlock()
sql.StatusVariables.IncrementGlobal("Threads_connected", 1)
pl.procs[id] = &sql.Process{
Connection: id,
Command: sql.ProcessCommandConnect,
Expand Down Expand Up @@ -116,7 +116,7 @@ func (pl *ProcessList) BeginQuery(
pl.mu.Lock()
defer pl.mu.Unlock()

sql.IncrementStatusVariable(ctx, "Threads_running", 1)
sql.StatusVariables.IncrementGlobal("Threads_running", 1)

id := ctx.Session.ID()
pid := ctx.Pid()
Expand Down Expand Up @@ -158,7 +158,7 @@ func (pl *ProcessList) EndQuery(ctx *sql.Context) {
sql.IncrementStatusVariable(ctx, "Slow_queries", 1)
}

sql.IncrementStatusVariable(ctx, "Threads_running", -1)
sql.StatusVariables.IncrementGlobal("Threads_running", -1)
p.Command = sql.ProcessCommandSleep
p.Query = ""
p.StartedAt = time.Now()
Expand Down
2 changes: 2 additions & 0 deletions processlist_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,12 @@ import (
"github.com/stretchr/testify/require"

"github.com/dolthub/go-mysql-server/sql"
"github.com/dolthub/go-mysql-server/sql/variables"
)

func TestProcessList(t *testing.T) {
require := require.New(t)
variables.InitStatusVariables()

clientHostOne := "127.0.0.1:34567"
clientHostTwo := "127.0.0.1:34568"
Expand Down
3 changes: 2 additions & 1 deletion server/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,8 @@ func (h *Handler) NewConnection(c *mysql.Conn) {
}

func (h *Handler) ConnectionAborted(_ *mysql.Conn, _ string) error {
return sql.StatusVariables.IncrementGlobal("Aborted_connects", 1)
sql.StatusVariables.IncrementGlobal("Aborted_connects", 1)
return nil
}

func (h *Handler) ComInitDB(c *mysql.Conn, schemaName string) error {
Expand Down
8 changes: 5 additions & 3 deletions sql/base_session.go
Original file line number Diff line number Diff line change
Expand Up @@ -235,17 +235,19 @@ func (s *BaseSession) GetAllStatusVariables(_ *Context) map[string]StatusVarValu
return m
}

var done bool
max-hoffman marked this conversation as resolved.
Show resolved Hide resolved

// IncrementStatusVariable implements the Session interface.
func (s *BaseSession) IncrementStatusVariable(_ *Context, statVarName string, val int) error {
func (s *BaseSession) IncrementStatusVariable(ctx *Context, statVarName string, val int) {
if _, ok := s.statusVars[statVarName]; !ok {
return ErrUnknownSystemVariable.New(statVarName)
return
}
if val < 0 {
s.statusVars[statVarName].Increment(-(uint64(-val)))
} else {
s.statusVars[statVarName].Increment((uint64(val)))
}
return nil
return
}

// GetCharacterSet returns the character set for this session (defined by the system variable `character_set_connection`).
Expand Down
11 changes: 4 additions & 7 deletions sql/core.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

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

Copy link
Contributor Author

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

IncrementGlobal(name string, val int) error
IncrementGlobal(name string, val int)
}

// StatusVariableScope represents the scope of a status variable.
Expand Down Expand Up @@ -833,11 +833,8 @@ func (s *ImmutableStatusVarValue) Copy() StatusVarValue {
}

// IncrementStatusVariable increments the value of the status variable by integer val.
// name is case-insensitive. Errors are ignored.
// This runs in a goroutine to avoid blocking the caller, but we do not wait for it to complete.
// |name| is case-sensitive.
func IncrementStatusVariable(ctx *Context, name string, val int) {
go func() {
StatusVariables.IncrementGlobal(name, val)
ctx.Session.IncrementStatusVariable(ctx, name, val)
}()
StatusVariables.IncrementGlobal(name, val)
ctx.Session.IncrementStatusVariable(ctx, name, val)
}
2 changes: 2 additions & 0 deletions sql/rowexec/show_status_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,13 @@ import (

"github.com/dolthub/go-mysql-server/sql"
"github.com/dolthub/go-mysql-server/sql/plan"
"github.com/dolthub/go-mysql-server/sql/variables"
)

func TestShowStatus(t *testing.T) {
require := require.New(t)
ctx := sql.NewEmptyContext()
variables.InitStatusVariables()

var res sql.Row
var err error
Expand Down
2 changes: 1 addition & 1 deletion sql/session.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ type Session interface {
// To access global scope, use sql.StatusVariables instead.
GetAllStatusVariables(ctx *Context) map[string]StatusVarValue
// IncrementStatusVariable increments the value of the status variable by the integer value
IncrementStatusVariable(ctx *Context, statVarName string, val int) error
IncrementStatusVariable(ctx *Context, statVarName string, val int)
// GetCurrentDatabase gets the current database for this session
GetCurrentDatabase() string
// SetCurrentDatabase sets the current database for this session
Expand Down
12 changes: 3 additions & 9 deletions sql/variables/status_variables.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,19 +93,13 @@ func (g *globalStatusVariables) SetGlobal(name string, val interface{}) error {
}

// IncrementGlobal implements sql.StatusVariableRegistry
func (g *globalStatusVariables) IncrementGlobal(name string, val int) error {
func (g *globalStatusVariables) IncrementGlobal(name string, val int) {
v, ok := g.varVals[name]
if !ok || v.Variable().GetScope() == sql.StatusVariableScope_Session {
return sql.ErrUnknownSystemVariable.New(name)
return
Copy link
Member

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?

Copy link
Contributor Author

@max-hoffman max-hoffman Aug 1, 2024

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.

Copy link
Contributor Author

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

}
g.varVals[name].Increment(uint64(val))
return nil
}

// init initializes SystemVariables as it functions as a global variable.
// TODO: get rid of me, make this construction the responsibility of the engine
func init() {
InitStatusVariables()
return
}

// InitStatusVariables initializes the global status variables in sql.StatusVariables. If they have already
Expand Down
1 change: 1 addition & 0 deletions sql/variables/status_variables_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
)

func TestStatusVariables(t *testing.T) {
InitStatusVariables()
defer InitStatusVariables()

require := require.New(t)
Expand Down