From 3a4c7603b3ce35ab247b4dddaf7a2506375ba2c9 Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Mon, 16 Apr 2018 21:23:20 -0400 Subject: MM-10020: avoid duplicating unique indexes (#8587) * unit test TestGet(Search)Replica This adds partial, testing-focused support for SQLite, as well as removing some translated log messages that required initializing i18n just for testing. * avoid returning master twice when no replicas are configured * remove duplicate indexes * unit test GetAllConns --- store/sqlstore/preference_store.go | 3 +- store/sqlstore/supplier.go | 43 ++++--- store/sqlstore/supplier_test.go | 229 +++++++++++++++++++++++++++++++++++++ store/sqlstore/upgrade.go | 6 +- 4 files changed, 265 insertions(+), 16 deletions(-) create mode 100644 store/sqlstore/supplier_test.go (limited to 'store/sqlstore') diff --git a/store/sqlstore/preference_store.go b/store/sqlstore/preference_store.go index a97c3aea7..791b51d2f 100644 --- a/store/sqlstore/preference_store.go +++ b/store/sqlstore/preference_store.go @@ -11,7 +11,6 @@ import ( "github.com/mattermost/mattermost-server/model" "github.com/mattermost/mattermost-server/store" - "github.com/mattermost/mattermost-server/utils" ) type SqlPreferenceStore struct { @@ -39,7 +38,7 @@ func (s SqlPreferenceStore) CreateIndexesIfNotExists() { } func (s SqlPreferenceStore) DeleteUnusedFeatures() { - l4g.Debug(utils.T("store.sql_preference.delete_unused_features.debug")) + l4g.Debug("Deleting any unused pre-release features") sql := `DELETE FROM Preferences diff --git a/store/sqlstore/supplier.go b/store/sqlstore/supplier.go index 5e43ee0f0..99b35a664 100644 --- a/store/sqlstore/supplier.go +++ b/store/sqlstore/supplier.go @@ -59,6 +59,8 @@ const ( EXIT_REMOVE_INDEX_MYSQL = 122 EXIT_REMOVE_INDEX_MISSING = 123 EXIT_REMOVE_TABLE = 134 + EXIT_CREATE_INDEX_SQLITE = 135 + EXIT_REMOVE_INDEX_SQLITE = 136 ) type SqlSupplierOldStores struct { @@ -185,7 +187,7 @@ func (s *SqlSupplier) Next() store.LayeredStoreSupplier { func setupConnection(con_type string, dataSource string, settings *model.SqlSettings) *gorp.DbMap { db, err := dbsql.Open(*settings.DriverName, dataSource) if err != nil { - l4g.Critical(utils.T("store.sql.open_conn.critical"), err) + l4g.Critical("Failed to open SQL connection to err:%v", err.Error()) time.Sleep(time.Second) os.Exit(EXIT_DB_OPEN) } @@ -217,7 +219,7 @@ func setupConnection(con_type string, dataSource string, settings *model.SqlSett connectionTimeout := time.Duration(*settings.QueryTimeout) * time.Second - if *settings.DriverName == "sqlite3" { + if *settings.DriverName == model.DATABASE_DRIVER_SQLITE { dbmap = &gorp.DbMap{Db: db, TypeConverter: mattermConverter{}, Dialect: gorp.SqliteDialect{}, QueryTimeout: connectionTimeout} } else if *settings.DriverName == model.DATABASE_DRIVER_MYSQL { dbmap = &gorp.DbMap{Db: db, TypeConverter: mattermConverter{}, Dialect: gorp.MySQLDialect{Engine: "InnoDB", Encoding: "UTF8MB4"}, QueryTimeout: connectionTimeout} @@ -239,19 +241,14 @@ func setupConnection(con_type string, dataSource string, settings *model.SqlSett func (s *SqlSupplier) initConnection() { s.master = setupConnection("master", *s.settings.DataSource, s.settings) - if len(s.settings.DataSourceReplicas) == 0 { - s.replicas = make([]*gorp.DbMap, 1) - s.replicas[0] = s.master - } else { + if len(s.settings.DataSourceReplicas) > 0 { s.replicas = make([]*gorp.DbMap, len(s.settings.DataSourceReplicas)) for i, replica := range s.settings.DataSourceReplicas { s.replicas[i] = setupConnection(fmt.Sprintf("replica-%v", i), replica, s.settings) } } - if len(s.settings.DataSourceSearchReplicas) == 0 { - s.searchReplicas = s.replicas - } else { + if len(s.settings.DataSourceSearchReplicas) > 0 { s.searchReplicas = make([]*gorp.DbMap, len(s.settings.DataSourceSearchReplicas)) for i, replica := range s.settings.DataSourceSearchReplicas { s.searchReplicas[i] = setupConnection(fmt.Sprintf("search-replica-%v", i), replica, s.settings) @@ -273,11 +270,19 @@ func (ss *SqlSupplier) GetMaster() *gorp.DbMap { } func (ss *SqlSupplier) GetSearchReplica() *gorp.DbMap { + if len(ss.settings.DataSourceSearchReplicas) == 0 { + return ss.GetReplica() + } + rrNum := atomic.AddInt64(&ss.srCounter, 1) % int64(len(ss.searchReplicas)) return ss.searchReplicas[rrNum] } func (ss *SqlSupplier) GetReplica() *gorp.DbMap { + if len(ss.settings.DataSourceReplicas) == 0 { + return ss.GetMaster() + } + rrNum := atomic.AddInt64(&ss.rrCounter, 1) % int64(len(ss.replicas)) return ss.replicas[rrNum] } @@ -287,7 +292,6 @@ func (ss *SqlSupplier) TotalMasterDbConnections() int { } func (ss *SqlSupplier) TotalReadDbConnections() int { - if len(ss.settings.DataSourceReplicas) == 0 { return 0 } @@ -598,8 +602,7 @@ func (ss *SqlSupplier) createIndexIfNotExists(indexName string, tableName string _, err := ss.GetMaster().ExecNoTimeout(query) if err != nil { - l4g.Critical(utils.T("store.sql.create_index.critical"), errExists) - l4g.Critical(utils.T("store.sql.create_index.critical"), err) + l4g.Critical("Failed to create index %v, %v", errExists, err) time.Sleep(time.Second) os.Exit(EXIT_CREATE_INDEX_POSTGRES) } @@ -623,10 +626,17 @@ func (ss *SqlSupplier) createIndexIfNotExists(indexName string, tableName string _, err = ss.GetMaster().ExecNoTimeout("CREATE " + uniqueStr + fullTextIndex + " INDEX " + indexName + " ON " + tableName + " (" + strings.Join(columnNames, ", ") + ")") if err != nil { - l4g.Critical(utils.T("store.sql.create_index.critical"), err) + l4g.Critical("Failed to create index %v", err) time.Sleep(time.Second) os.Exit(EXIT_CREATE_INDEX_FULL_MYSQL) } + } else if ss.DriverName() == model.DATABASE_DRIVER_SQLITE { + _, err := ss.GetMaster().ExecNoTimeout("CREATE INDEX IF NOT EXISTS " + indexName + " ON " + tableName + " (" + strings.Join(columnNames, ", ") + ")") + if err != nil { + l4g.Critical("Failed to create index %v", err) + time.Sleep(time.Second) + os.Exit(EXIT_CREATE_INDEX_SQLITE) + } } else { l4g.Critical(utils.T("store.sql.create_index_missing_driver.critical")) time.Sleep(time.Second) @@ -672,6 +682,13 @@ func (ss *SqlSupplier) RemoveIndexIfExists(indexName string, tableName string) b time.Sleep(time.Second) os.Exit(EXIT_REMOVE_INDEX_MYSQL) } + } else if ss.DriverName() == model.DATABASE_DRIVER_SQLITE { + _, err := ss.GetMaster().ExecNoTimeout("DROP INDEX IF EXISTS " + indexName) + if err != nil { + l4g.Critical(utils.T("store.sql.remove_index.critical"), err) + time.Sleep(time.Second) + os.Exit(EXIT_REMOVE_INDEX_SQLITE) + } } else { l4g.Critical(utils.T("store.sql.create_index_missing_driver.critical")) time.Sleep(time.Second) diff --git a/store/sqlstore/supplier_test.go b/store/sqlstore/supplier_test.go new file mode 100644 index 000000000..5aacfbe6a --- /dev/null +++ b/store/sqlstore/supplier_test.go @@ -0,0 +1,229 @@ +package sqlstore_test + +import ( + "testing" + + "github.com/mattermost/gorp" + _ "github.com/mattn/go-sqlite3" + "github.com/stretchr/testify/assert" + + "github.com/mattermost/mattermost-server/model" + "github.com/mattermost/mattermost-server/store/sqlstore" +) + +func TestGetReplica(t *testing.T) { + t.Parallel() + testCases := []struct { + Description string + DataSourceReplicas []string + DataSourceSearchReplicas []string + }{ + { + "no replicas", + []string{}, + []string{}, + }, + { + "one source replica", + []string{":memory:"}, + []string{}, + }, + { + "multiple source replicas", + []string{":memory:", ":memory:", ":memory:"}, + []string{}, + }, + { + "one source search replica", + []string{}, + []string{":memory:"}, + }, + { + "multiple source search replicas", + []string{}, + []string{":memory:", ":memory:", ":memory:"}, + }, + { + "one source replica, one source search replica", + []string{":memory:"}, + []string{":memory:"}, + }, + { + "one source replica, multiple source search replicas", + []string{":memory:"}, + []string{":memory:", ":memory:", ":memory:"}, + }, + { + "multiple source replica, one source search replica", + []string{":memory:", ":memory:", ":memory:"}, + []string{":memory:"}, + }, + { + "multiple source replica, multiple source search replicas", + []string{":memory:", ":memory:", ":memory:"}, + []string{":memory:", ":memory:", ":memory:"}, + }, + } + + for _, testCase := range testCases { + testCase := testCase + t.Run(testCase.Description, func(t *testing.T) { + t.Parallel() + + driverName := model.DATABASE_DRIVER_SQLITE + dataSource := ":memory:" + maxIdleConns := 1 + maxOpenConns := 1 + queryTimeout := 5 + + settings := model.SqlSettings{ + DriverName: &driverName, + DataSource: &dataSource, + MaxIdleConns: &maxIdleConns, + MaxOpenConns: &maxOpenConns, + QueryTimeout: &queryTimeout, + DataSourceReplicas: testCase.DataSourceReplicas, + DataSourceSearchReplicas: testCase.DataSourceSearchReplicas, + } + supplier := sqlstore.NewSqlSupplier(settings, nil) + + replicas := make(map[*gorp.DbMap]bool) + for i := 0; i < 5; i++ { + replicas[supplier.GetReplica()] = true + } + + searchReplicas := make(map[*gorp.DbMap]bool) + for i := 0; i < 5; i++ { + searchReplicas[supplier.GetSearchReplica()] = true + } + + if len(testCase.DataSourceReplicas) > 0 { + // If replicas were defined, ensure none are the master. + assert.Len(t, replicas, len(testCase.DataSourceReplicas)) + + for replica := range replicas { + assert.NotEqual(t, supplier.GetMaster(), replica) + } + + } else if assert.Len(t, replicas, 1) { + // Otherwise ensure the replicas contains only the master. + for replica := range replicas { + assert.Equal(t, supplier.GetMaster(), replica) + } + } + + if len(testCase.DataSourceSearchReplicas) > 0 { + // If search replicas were defined, ensure none are the master nor the replicas. + assert.Len(t, searchReplicas, len(testCase.DataSourceSearchReplicas)) + + for searchReplica := range searchReplicas { + assert.NotEqual(t, supplier.GetMaster(), searchReplica) + for replica := range replicas { + assert.NotEqual(t, searchReplica, replica) + } + } + + } else if len(testCase.DataSourceReplicas) > 0 { + // If no search replicas were defined, but replicas were, ensure they are equal. + assert.Equal(t, replicas, searchReplicas) + + } else if assert.Len(t, searchReplicas, 1) { + // Otherwise ensure the search replicas contains the master. + for searchReplica := range searchReplicas { + assert.Equal(t, supplier.GetMaster(), searchReplica) + } + } + }) + } +} + +func TestGetAllConns(t *testing.T) { + t.Parallel() + testCases := []struct { + Description string + DataSourceReplicas []string + DataSourceSearchReplicas []string + ExpectedNumConnections int + }{ + { + "no replicas", + []string{}, + []string{}, + 1, + }, + { + "one source replica", + []string{":memory:"}, + []string{}, + 2, + }, + { + "multiple source replicas", + []string{":memory:", ":memory:", ":memory:"}, + []string{}, + 4, + }, + { + "one source search replica", + []string{}, + []string{":memory:"}, + 1, + }, + { + "multiple source search replicas", + []string{}, + []string{":memory:", ":memory:", ":memory:"}, + 1, + }, + { + "one source replica, one source search replica", + []string{":memory:"}, + []string{":memory:"}, + 2, + }, + { + "one source replica, multiple source search replicas", + []string{":memory:"}, + []string{":memory:", ":memory:", ":memory:"}, + 2, + }, + { + "multiple source replica, one source search replica", + []string{":memory:", ":memory:", ":memory:"}, + []string{":memory:"}, + 4, + }, + { + "multiple source replica, multiple source search replicas", + []string{":memory:", ":memory:", ":memory:"}, + []string{":memory:", ":memory:", ":memory:"}, + 4, + }, + } + + for _, testCase := range testCases { + testCase := testCase + t.Run(testCase.Description, func(t *testing.T) { + t.Parallel() + + driverName := model.DATABASE_DRIVER_SQLITE + dataSource := ":memory:" + maxIdleConns := 1 + maxOpenConns := 1 + queryTimeout := 5 + + settings := model.SqlSettings{ + DriverName: &driverName, + DataSource: &dataSource, + MaxIdleConns: &maxIdleConns, + MaxOpenConns: &maxOpenConns, + QueryTimeout: &queryTimeout, + DataSourceReplicas: testCase.DataSourceReplicas, + DataSourceSearchReplicas: testCase.DataSourceSearchReplicas, + } + supplier := sqlstore.NewSqlSupplier(settings, nil) + + assert.Equal(t, testCase.ExpectedNumConnections, len(supplier.GetAllConns())) + }) + } +} diff --git a/store/sqlstore/upgrade.go b/store/sqlstore/upgrade.go index 882ee48ba..059d1a866 100644 --- a/store/sqlstore/upgrade.go +++ b/store/sqlstore/upgrade.go @@ -87,7 +87,7 @@ func UpgradeDatabase(sqlStore SqlStore) { os.Exit(EXIT_VERSION_SAVE_MISSING) } - l4g.Info(utils.T("store.sql.schema_set.info"), model.CurrentVersion) + l4g.Info("The database schema has been set to version %v", model.CurrentVersion) } // If we're not on the current version then it's too old to be upgraded @@ -415,6 +415,10 @@ func UpgradeDatabaseToVersion410(sqlStore SqlStore) { // TODO: Uncomment following condition when version 4.10.0 is released //if shouldPerformUpgrade(sqlStore, VERSION_4_9_0, VERSION_4_10_0) { + sqlStore.RemoveIndexIfExists("Name_2", "Channels") + sqlStore.RemoveIndexIfExists("Name_2", "Emoji") + sqlStore.RemoveIndexIfExists("ClientId_2", "OAuthAccessData") + // saveSchemaVersion(sqlStore, VERSION_4_10_0) //} } -- cgit v1.2.3-1-g7c22