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

@@ -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
 }

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

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

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

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)

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) {