turn on stats collect by default (#8453)

* turn on stats collect by default * better impl of default stats for server only * [ga-format-pr] Run go/utils/repofmt/format_repo.sh and go/Godeps/update.sh * fix deadlock * [ga-format-pr] Run go/utils/repofmt/format_repo.sh and go/Godeps/update.sh * split off branch delete interface * fix more tests * fix more tests * add test for auto-on * zach comments --------- Co-authored-by: max-hoffman <[email protected]>
dolthub · Oct 24, 2024 · 2d50424 · 2d50424
1 parent fe54813
commit 2d50424
Show file tree

Hide file tree

Showing 10 changed files with 83 additions and 27 deletions.
diff --git a/go/cmd/dolt/commands/sqlserver/server.go b/go/cmd/dolt/commands/sqlserver/server.go
@@ -271,8 +271,16 @@ func ConfigureServices(
 	var sqlEngine *engine.SqlEngine
 	InitSqlEngine := &svcs.AnonService{
 		InitF: func(ctx context.Context) (err error) {
-			if _, err := mrEnv.Config().GetString(env.SqlServerGlobalsPrefix + "." + dsess.DoltStatsBootstrapEnabled); err != nil {
-				// unless otherwise specified by config, enable server bootstrapping
+			if statsOn, err := mrEnv.Config().GetString(env.SqlServerGlobalsPrefix + "." + dsess.DoltStatsAutoRefreshEnabled); err != nil {
+				// Auto-stats is off by default for every command except
+				// sql-server. Unless the config specifies a specific
+				// behavior, enable server stats collection.
+				sql.SystemVariables.SetGlobal(dsess.DoltStatsAutoRefreshEnabled, 1)
+			} else if statsOn != "0" {
+				// do not bootstrap if auto-stats enabled
+			} else if _, err := mrEnv.Config().GetString(env.SqlServerGlobalsPrefix + "." + dsess.DoltStatsBootstrapEnabled); err != nil {
+				// If we've disabled stats collection and config does not
+				// specify bootstrap behavior, enable bootstrapping.
 				sql.SystemVariables.SetGlobal(dsess.DoltStatsBootstrapEnabled, 1)
 			}
 			sqlEngine, err = engine.NewSqlEngine(

diff --git a/go/libraries/doltcore/sqle/cluster/controller.go b/go/libraries/doltcore/sqle/cluster/controller.go
@@ -423,7 +423,7 @@ func (c *Controller) replicateDropDatabase(s *databaseDropReplication, client *r
 			return
 		}
 		if status.Code(err) == codes.FailedPrecondition {
-			c.lgr.Warnf("drop of [%s] to %s will note be replicated; FailedPrecondition", dbname, client.remote)
+			c.lgr.Warnf("drop of [%s] to %s will not be replicated; FailedPrecondition", dbname, client.remote)
 			return
 		}
 		c.lgr.Warnf("failed to replicate drop of [%s] to %s: %v", dbname, client.remote, err)

diff --git a/go/libraries/doltcore/sqle/dprocedures/stats_funcs.go b/go/libraries/doltcore/sqle/dprocedures/stats_funcs.go
@@ -55,6 +55,10 @@ type AutoRefreshStatsProvider interface {
 	Purge(ctx *sql.Context) error
 }
 
+type BranchStatsProvider interface {
+	DropBranchDbStats(ctx *sql.Context, branch, db string, flush bool) error
+}
+
 // statsRestart tries to stop and then start a refresh thread
 func statsRestart(ctx *sql.Context) (interface{}, error) {
 	dSess := dsess.DSessFromSess(ctx.Session)
@@ -116,14 +120,22 @@ func statsDrop(ctx *sql.Context) (interface{}, error) {
 	pro := dSess.StatsProvider()
 	dbName := strings.ToLower(ctx.GetCurrentDatabase())
 
+	branch, err := dSess.GetBranch()
+	if err != nil {
+		return nil, fmt.Errorf("failed to drop stats: %w", err)
+	}
+
 	if afp, ok := pro.(AutoRefreshStatsProvider); ok {
 		// currently unsafe to drop stats while running refresh
 		afp.CancelRefreshThread(dbName)
 	}
-	err := pro.DropDbStats(ctx, dbName, true)
-	if err != nil {
-		return nil, fmt.Errorf("failed to drop stats: %w", err)
+	if bsp, ok := pro.(BranchStatsProvider); ok {
+		err := bsp.DropBranchDbStats(ctx, branch, dbName, true)
+		if err != nil {
+			return nil, fmt.Errorf("failed to drop stats: %w", err)
+		}
 	}
+
 	return fmt.Sprintf("deleted stats ref for %s", dbName), nil
 }
 

diff --git a/go/libraries/doltcore/sqle/enginetest/dolt_harness.go b/go/libraries/doltcore/sqle/enginetest/dolt_harness.go
@@ -495,6 +495,7 @@ func (d *DoltHarness) NewDatabaseProvider() sql.MutableDatabaseProvider {
 
 func (d *DoltHarness) Close() {
 	d.closeProvider()
+	sql.SystemVariables.SetGlobal(dsess.DoltStatsAutoRefreshEnabled, int8(0))
 }
 
 func (d *DoltHarness) closeProvider() {

diff --git a/go/libraries/doltcore/sqle/statspro/configure.go b/go/libraries/doltcore/sqle/statspro/configure.go
@@ -24,6 +24,7 @@ import (
 	types2 "github.com/dolthub/go-mysql-server/sql/types"
 
 	"github.com/dolthub/dolt/go/libraries/doltcore/env"
+	"github.com/dolthub/dolt/go/libraries/doltcore/sqle"
 	"github.com/dolthub/dolt/go/libraries/doltcore/sqle/dsess"
 	"github.com/dolthub/dolt/go/libraries/utils/filesys"
 )
@@ -55,7 +56,7 @@ func (p *Provider) Configure(ctx context.Context, ctxFactory func(ctx context.Co
 		thresholdf64 = threshold.(float64)
 
 		p.pro.InitDatabaseHooks = append(p.pro.InitDatabaseHooks, NewStatsInitDatabaseHook(p, ctxFactory, bThreads))
-		p.pro.DropDatabaseHooks = append(p.pro.DropDatabaseHooks, NewStatsDropDatabaseHook(p))
+		p.pro.DropDatabaseHooks = append([]sqle.DropDatabaseHook{NewStatsDropDatabaseHook(p)}, p.pro.DropDatabaseHooks...)
 	} else if _, startupStats, _ := sql.SystemVariables.GetGlobal(dsess.DoltStatsBootstrapEnabled); startupStats == int8(1) {
 		startupEnabled = true
 	}

diff --git a/go/libraries/doltcore/sqle/statspro/initdbhook.go b/go/libraries/doltcore/sqle/statspro/initdbhook.go
@@ -54,7 +54,9 @@ func NewStatsInitDatabaseHook(
 func NewStatsDropDatabaseHook(statsProv *Provider) sqle.DropDatabaseHook {
 	return func(ctx *sql.Context, name string) {
 		statsProv.CancelRefreshThread(name)
-		statsProv.DropDbStats(ctx, name, false)
+		if err := statsProv.DropDbStats(ctx, name, false); err != nil {
+			ctx.GetLogger().Debugf("failed to close stats database: %s", err)
+		}
 
 		if db, ok := statsProv.getStatDb(name); ok {
 			if err := db.Close(); err != nil {

diff --git a/go/libraries/doltcore/sqle/statspro/interface.go b/go/libraries/doltcore/sqle/statspro/interface.go
@@ -50,7 +50,6 @@ type Database interface {
 	Flush(ctx context.Context, branch string) error
 	// Close finalizes any file references.
 	Close() error
-
 	SetLatestHash(branch, tableName string, h hash.Hash)
 	GetLatestHash(branch, tableName string) hash.Hash
 	Branches() []string

diff --git a/go/libraries/doltcore/sqle/statspro/stats_provider.go b/go/libraries/doltcore/sqle/statspro/stats_provider.go
@@ -212,6 +212,12 @@ func (p *Provider) getStatDb(name string) (Database, bool) {
 	return statDb, ok
 }
 
+func (p *Provider) deleteStatDb(name string) {
+	p.mu.Lock()
+	defer p.mu.Unlock()
+	delete(p.statDbs, strings.ToLower(name))
+}
+
 func (p *Provider) SetStats(ctx *sql.Context, s sql.Statistic) error {
 	statDb, ok := p.getStatDb(s.Qualifier().Db())
 	if !ok {
@@ -257,7 +263,7 @@ func (p *Provider) GetStats(ctx *sql.Context, qual sql.StatQualifier, _ []string
 	return stat, true
 }
 
-func (p *Provider) DropDbBranchStats(ctx *sql.Context, branch, db string, flush bool) error {
+func (p *Provider) DropBranchDbStats(ctx *sql.Context, branch, db string, flush bool) error {
 	statDb, ok := p.getStatDb(db)
 	if !ok {
 		return nil
@@ -266,24 +272,26 @@ func (p *Provider) DropDbBranchStats(ctx *sql.Context, branch, db string, flush
 	p.mu.Lock()
 	defer p.mu.Unlock()
 
-	// remove provider access
-	if err := statDb.DeleteBranchStats(ctx, branch, flush); err != nil {
-		return nil
-	}
-
 	p.status[db] = "dropped"
 
-	return nil
+	return statDb.DeleteBranchStats(ctx, branch, flush)
 }
 
 func (p *Provider) DropDbStats(ctx *sql.Context, db string, flush bool) error {
-	dSess := dsess.DSessFromSess(ctx.Session)
-	branch, err := dSess.GetBranch()
-	if err != nil {
-		return err
+	statDb, ok := p.getStatDb(db)
+	if !ok {
+		return nil
+	}
+	for _, branch := range statDb.Branches() {
+		// remove provider access
+		p.DropBranchDbStats(ctx, branch, db, flush)
+	}
+
+	if flush {
+		p.deleteStatDb(db)
 	}
 
-	return p.DropDbBranchStats(ctx, branch, db, flush)
+	return nil
 }
 
 func (p *Provider) DropStats(ctx *sql.Context, qual sql.StatQualifier, _ []string) error {
@@ -403,7 +411,7 @@ func (p *Provider) Prune(ctx *sql.Context) error {
 					stats = append(stats, tableStats...)
 				}
 
-				if err := p.DropDbBranchStats(ctx, branch, dbName, true); err != nil {
+				if err := p.DropBranchDbStats(ctx, branch, dbName, true); err != nil {
 					return err
 				}
 
@@ -457,7 +465,7 @@ func (p *Provider) Purge(ctx *sql.Context) error {
 						defer p.UnlockTable(branch, dbName, t)
 					}
 
-					err := p.DropDbBranchStats(ctx, branch, dbName, true)
+					err := p.DropBranchDbStats(ctx, branch, dbName, true)
 					if err != nil {
 						return fmt.Errorf("failed to drop stats: %w", err)
 					}

diff --git a/go/libraries/doltcore/sqle/system_variables.go b/go/libraries/doltcore/sqle/system_variables.go
@@ -236,7 +236,7 @@ var DoltSystemVariables = []sql.SystemVariable{
 		Name:    dsess.DoltStatsAutoRefreshThreshold,
 		Dynamic: true,
 		Scope:   sql.GetMysqlScope(sql.SystemVariableScope_Global),
-		Type:    types.NewSystemDoubleType(dsess.DoltStatsAutoRefreshEnabled, 0, 10),
+		Type:    types.NewSystemDoubleType(dsess.DoltStatsAutoRefreshThreshold, 0, 10),
 		Default: float64(.5),
 	},
 	&sql.MysqlSystemVariable{
@@ -463,7 +463,7 @@ func AddDoltSystemVariables() {
 			Name:    dsess.DoltStatsAutoRefreshThreshold,
 			Dynamic: true,
 			Scope:   sql.GetMysqlScope(sql.SystemVariableScope_Global),
-			Type:    types.NewSystemDoubleType(dsess.DoltStatsAutoRefreshEnabled, 0, 10),
+			Type:    types.NewSystemDoubleType(dsess.DoltStatsAutoRefreshThreshold, 0, 10),
 			Default: float64(.5),
 		},
 		&sql.MysqlSystemVariable{

diff --git a/integration-tests/bats/stats.bats b/integration-tests/bats/stats.bats
@@ -91,9 +91,12 @@ teardown() {
     [ "${lines[1]}" = "8" ]
 }
 
-@test "stats: bootrap on server startup" {
+@test "stats: bootstrap on server startup" {
     cd repo2
 
+    # disable higher precedence auto-update
+    dolt sql -q "set @@PERSIST.dolt_stats_auto_refresh_enabled = 0;"
+
     dolt sql -q "insert into xy values (0,0), (1,1)"
 
     start_sql_server
@@ -104,7 +107,29 @@ teardown() {
     [ "${lines[1]}" = "2" ]
 }
 
-@test "stats: only bootrap server startup" {
+@test "stats: auto-update on server startup" {
+    cd repo2
+
+    dolt sql -q "set @@PERSIST.dolt_stats_auto_refresh_enabled = 1;"
+    dolt sql -q "set @@PERSIST.dolt_stats_auto_refresh_threshold = 0"
+    dolt sql -q "set @@PERSIST.dolt_stats_auto_refresh_interval = 0;"
+
+    run dolt sql -r csv -q "select count(*) from dolt_statistics"
+    [ "$status" -eq 0 ]
+    [ "${lines[1]}" = "0" ]
+
+    start_sql_server
+    dolt sql -q "insert into xy values (0,0), (1,1)"
+    sleep 1
+    stop_sql_server
+
+    run dolt sql -r csv -q "select count(*) from dolt_statistics"
+    [ "$status" -eq 0 ]
+    [ "${lines[1]}" = "2" ]
+}
+
+
+@test "stats: only bootstrap server startup" {
     cd repo2
 
     dolt sql -q "insert into xy values (0,0), (1,1)"