From 43e6e261d66cad4bbb0d7ddcec19f6d8e7247949 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jes=C3=BAs=20Espino?= Date: Mon, 4 Mar 2019 17:52:26 +0100 Subject: [PATCH] MM-14357: Remove store call that can provoke dead locks (#10393) * MM-14357: Remove store call that can provoke dead locks * Simplify query * Adding unit test --- app/import.go | 4 --- store/sqlstore/channel_store.go | 39 --------------------------- store/sqlstore/post_store.go | 8 +++--- store/store.go | 1 - store/storetest/mocks/ChannelStore.go | 16 ----------- store/storetest/post_store.go | 14 ++++++++++ 6 files changed, 18 insertions(+), 64 deletions(-) diff --git a/app/import.go b/app/import.go index 468f9e505b..6ae76bc7e9 100644 --- a/app/import.go +++ b/app/import.go @@ -167,9 +167,5 @@ func (a *App) finalizeImport(dryRun bool) *model.AppError { if dryRun { return nil } - result := <-a.Srv.Store.Channel().ResetLastPostAt() - if result.Err != nil { - return result.Err - } return nil } diff --git a/store/sqlstore/channel_store.go b/store/sqlstore/channel_store.go index 6b572fe5ed..518328c8b3 100644 --- a/store/sqlstore/channel_store.go +++ b/store/sqlstore/channel_store.go @@ -2449,45 +2449,6 @@ func (s SqlChannelStore) ClearAllCustomRoleAssignments() store.StoreChannel { }) } -func (s SqlChannelStore) ResetLastPostAt() store.StoreChannel { - return store.Do(func(result *store.StoreResult) { - transaction, err := s.GetMaster().Begin() - if err != nil { - result.Err = model.NewAppError("SqlChannelStore.ResetLastPostAt", "store.sql_channel.reset_last_post_at.open_transaction.app_error", nil, err.Error(), http.StatusInternalServerError) - return - } - defer finalizeTransaction(transaction) - - *result = s.resetLastPostAtT(transaction) - if result.Err != nil { - return - } - - if err := transaction.Commit(); err != nil { - result.Err = model.NewAppError("SqlChannelStore.ResetLastPostAt", "store.sql_channel.reset_last_post_at.commit_transaction.app_error", nil, err.Error(), http.StatusInternalServerError) - return - } - }) -} - -func (s SqlChannelStore) resetLastPostAtT(transaction *gorp.Transaction) store.StoreResult { - result := store.StoreResult{} - - var query string - if s.DriverName() == model.DATABASE_DRIVER_POSTGRES { - query = "UPDATE Channels SET LastPostAt = COALESCE((SELECT UpdateAt FROM Posts WHERE ChannelId = Channels.Id ORDER BY UpdateAt DESC LIMIT 1), Channels.CreateAt);" - } else { - query = "UPDATE Channels SET LastPostAt = IFNULL((SELECT UpdateAt FROM Posts WHERE ChannelId = Channels.Id ORDER BY UpdateAt DESC LIMIT 1), Channels.CreateAt);" - } - - if _, err := transaction.Exec(query); err != nil { - result.Err = model.NewAppError("SqlChannelStore.ResetLastPostAt", "store.sql_channel.reset_last_post_at.app_error", nil, err.Error(), http.StatusInternalServerError) - return result - } - - return result -} - func (s SqlChannelStore) GetAllChannelsForExportAfter(limit int, afterId string) store.StoreChannel { return store.Do(func(result *store.StoreResult) { var data []*model.ChannelForExport diff --git a/store/sqlstore/post_store.go b/store/sqlstore/post_store.go index 9359e3061a..f9bfd35a4a 100644 --- a/store/sqlstore/post_store.go +++ b/store/sqlstore/post_store.go @@ -119,10 +119,10 @@ func (s *SqlPostStore) Save(post *model.Post) store.StoreChannel { post.Type != model.POST_JOIN_TEAM && post.Type != model.POST_LEAVE_TEAM && post.Type != model.POST_ADD_TO_CHANNEL && post.Type != model.POST_REMOVE_FROM_CHANNEL && post.Type != model.POST_ADD_TO_TEAM && post.Type != model.POST_REMOVE_FROM_TEAM { - s.GetMaster().Exec("UPDATE Channels SET LastPostAt = :LastPostAt, TotalMsgCount = TotalMsgCount + 1 WHERE Id = :ChannelId", map[string]interface{}{"LastPostAt": time, "ChannelId": post.ChannelId}) + s.GetMaster().Exec("UPDATE Channels SET LastPostAt = GREATEST(:LastPostAt, LastPostAt), TotalMsgCount = TotalMsgCount + 1 WHERE Id = :ChannelId", map[string]interface{}{"LastPostAt": time, "ChannelId": post.ChannelId}) } else { // don't update TotalMsgCount for unimportant messages so that the channel isn't marked as unread - s.GetMaster().Exec("UPDATE Channels SET LastPostAt = :LastPostAt WHERE Id = :ChannelId", map[string]interface{}{"LastPostAt": time, "ChannelId": post.ChannelId}) + s.GetMaster().Exec("UPDATE Channels SET LastPostAt = :LastPostAt WHERE Id = :ChannelId AND LastPostAt < :LastPostAt", map[string]interface{}{"LastPostAt": time, "ChannelId": post.ChannelId}) } if len(post.RootId) > 0 { @@ -161,10 +161,10 @@ func (s *SqlPostStore) Update(newPost *model.Post, oldPost *model.Post) store.St result.Err = model.NewAppError("SqlPostStore.Update", "store.sql_post.update.app_error", nil, "id="+newPost.Id+", "+err.Error(), http.StatusInternalServerError) } else { time := model.GetMillis() - s.GetMaster().Exec("UPDATE Channels SET LastPostAt = :LastPostAt WHERE Id = :ChannelId", map[string]interface{}{"LastPostAt": time, "ChannelId": newPost.ChannelId}) + s.GetMaster().Exec("UPDATE Channels SET LastPostAt = :LastPostAt WHERE Id = :ChannelId AND LastPostAt < :LastPostAt", map[string]interface{}{"LastPostAt": time, "ChannelId": newPost.ChannelId}) if len(newPost.RootId) > 0 { - s.GetMaster().Exec("UPDATE Posts SET UpdateAt = :UpdateAt WHERE Id = :RootId", map[string]interface{}{"UpdateAt": time, "RootId": newPost.RootId}) + s.GetMaster().Exec("UPDATE Posts SET UpdateAt = :UpdateAt WHERE Id = :RootId AND UpdateAt < :UpdateAt", map[string]interface{}{"UpdateAt": time, "RootId": newPost.RootId}) } // mark the old post as deleted diff --git a/store/store.go b/store/store.go index 92c439cabe..7db4149169 100644 --- a/store/store.go +++ b/store/store.go @@ -184,7 +184,6 @@ type ChannelStore interface { MigrateChannelMembers(fromChannelId string, fromUserId string) StoreChannel ResetAllChannelSchemes() StoreChannel ClearAllCustomRoleAssignments() StoreChannel - ResetLastPostAt() StoreChannel MigratePublicChannels() error GetAllChannelsForExportAfter(limit int, afterId string) StoreChannel GetChannelMembersForExport(userId string, teamId string) StoreChannel diff --git a/store/storetest/mocks/ChannelStore.go b/store/storetest/mocks/ChannelStore.go index 57bd1053d3..5a202bb34d 100644 --- a/store/storetest/mocks/ChannelStore.go +++ b/store/storetest/mocks/ChannelStore.go @@ -837,22 +837,6 @@ func (_m *ChannelStore) ResetAllChannelSchemes() store.StoreChannel { return r0 } -// ResetLastPostAt provides a mock function with given fields: -func (_m *ChannelStore) ResetLastPostAt() store.StoreChannel { - ret := _m.Called() - - var r0 store.StoreChannel - if rf, ok := ret.Get(0).(func() store.StoreChannel); ok { - r0 = rf() - } else { - if ret.Get(0) != nil { - r0 = ret.Get(0).(store.StoreChannel) - } - } - - return r0 -} - // Restore provides a mock function with given fields: channelId, time func (_m *ChannelStore) Restore(channelId string, time int64) store.StoreChannel { ret := _m.Called(channelId, time) diff --git a/store/storetest/post_store.go b/store/storetest/post_store.go index bac6ba3173..a51b2f4baa 100644 --- a/store/storetest/post_store.go +++ b/store/storetest/post_store.go @@ -93,6 +93,20 @@ func testPostStoreSaveChannelMsgCounts(t *testing.T, ss store.Store) { require.Nil(t, res.Err) c1 = res.Data.(*model.Channel) assert.Equal(t, int64(1), c1.TotalMsgCount, "Message count should not update for team add/removed message") + + oldLastPostAt := c1.LastPostAt + + o2 := model.Post{} + o2.ChannelId = c1.Id + o2.UserId = model.NewId() + o2.Message = "zz" + model.NewId() + "b" + o2.CreateAt = int64(7) + require.Nil(t, (<-ss.Post().Save(&o2)).Err) + + res = <-ss.Channel().Get(c1.Id, false) + require.Nil(t, res.Err) + c1 = res.Data.(*model.Channel) + assert.Equal(t, oldLastPostAt, c1.LastPostAt, "LastPostAt should not update for old message save") } func testPostStoreGet(t *testing.T, ss store.Store) {