MM-55295: Improve DB performance of some APIs (#25318)

Load tests show that channelstore.GetMember and
channelstore.GetMembersForUser are among the chief
queries that take up CPU in the DB.

In this PR, we attempt some strategic optimizations to
reduce/optimize calls to channelstore.GetMember

1. Optimize  `(a *App) HasPermissionToChannel`

We replace GetChannelMember with GetAllChannelMembersForUser
because it's cache backed. So although it gets more data,
it does not hit the DB and saves some latency.

2. Optimize getPostsForChannelAroundLastUnread

We repace getChannelMember with getChannelMemberOnly
which is a lite version of the store call which queries
just the ChannelMembers table. This is because
in the app layer, we just use the LastViewedAt attribute.
Therefore, there is no reason to join with 5 tables when
a single table can do the job.

3. Optimize publishWebsocketEventForPermalinkPost

We use GetAllChannelMembersById instead of GetChannelMembersPage
which again joins with a lot of other tables.

4. Optimize countMentionsFromPost

Again, we replace GetChannelMember which is a costly call joining
multiple tables, with GetAllChannelMembersNotifyPropsForChannel
which is cache-backed and gives us just what we need in the app
layer - notify props.

```release-note
Make small optimizations in several DB calls:
- App.HasPermissionToChannel
- getPostsForChannelAroundLastUnread
- publishWebsocketEventForPermalinkPost
- countMentionsFromPost
```

https://mattermost.atlassian.net/browse/MM-55295

---------

Co-authored-by: Mattermost Build <build@mattermost.com>
This commit is contained in:
Agniva De Sarker 2023-11-13 21:15:57 +05:30 committed by GitHub
parent 1ed5d87342
commit ec88ab4ee9
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
12 changed files with 189 additions and 53 deletions

View file

@ -101,7 +101,6 @@ func (a *App) SessionHasPermissionToChannel(c request.CTX, session model.Session
}
ids, err := a.Srv().Store().Channel().GetAllChannelMembersForUser(session.UserId, true, true)
var channelRoles []string
if err == nil {
if roles, ok := ids[channelID]; ok {
@ -299,17 +298,22 @@ func (a *App) HasPermissionToChannel(c request.CTX, askingUserId string, channel
return false
}
channelMember, err := a.GetChannelMember(c, channelID, askingUserId)
// We call GetAllChannelMembersForUser instead of just getting
// a single member from the DB, because it's cache backed
// and this is a very frequent call.
ids, err := a.Srv().Store().Channel().GetAllChannelMembersForUser(askingUserId, true, true)
var channelRoles []string
if err == nil {
roles := channelMember.GetRoles()
if a.RolesGrantPermission(roles, permission.Id) {
return true
if roles, ok := ids[channelID]; ok {
channelRoles = strings.Fields(roles)
if a.RolesGrantPermission(channelRoles, permission.Id) {
return true
}
}
}
var channel *model.Channel
channel, err = a.GetChannel(c, channelID)
if err == nil {
channel, appErr := a.GetChannel(c, channelID)
if appErr == nil {
return a.HasPermissionToTeam(c, askingUserId, channel.TeamId, permission)
}

View file

@ -2063,6 +2063,21 @@ func (s *Server) getChannelMember(c request.CTX, channelID string, userID string
return channelMember, nil
}
func (s *Server) getChannelMemberOnly(c request.CTX, channelID string, userID string) (*model.ChannelMember, *model.AppError) {
channelMember, err := s.Store().Channel().GetMemberOnly(c.Context(), channelID, userID)
if err != nil {
var nfErr *store.ErrNotFound
switch {
case errors.As(err, &nfErr):
return nil, model.NewAppError("getChannelMemberOnly", MissingChannelMemberError, nil, "", http.StatusNotFound).Wrap(err)
default:
return nil, model.NewAppError("getChannelMemberOnly", "app.channel.get_member.app_error", nil, "", http.StatusInternalServerError).Wrap(err)
}
}
return channelMember, nil
}
func (a *App) GetChannelMembersPage(c request.CTX, channelID string, page, perPage int) (model.ChannelMembers, *model.AppError) {
channelMembers, err := a.Srv().Store().Channel().GetMembers(channelID, page*perPage, perPage)
if err != nil {

View file

@ -785,9 +785,9 @@ func (a *App) publishWebsocketEventForPermalinkPost(c request.CTX, post *model.P
return false, err
}
channelMembers, err := a.GetChannelMembersPage(c, post.ChannelId, 0, 10000000)
if err != nil {
return false, err
userIDs, nErr := a.Srv().Store().Channel().GetAllChannelMemberIdsByChannelId(post.ChannelId)
if nErr != nil {
return false, model.NewAppError("publishWebsocketEventForPermalinkPost", "app.channel.get_members.app_error", nil, "", http.StatusInternalServerError).Wrap(err)
}
permalinkPreviewedChannel, err := a.GetChannel(c, previewedPost.ChannelId)
@ -800,18 +800,18 @@ func (a *App) publishWebsocketEventForPermalinkPost(c request.CTX, post *model.P
}
permalinkPreviewedPost := post.GetPreviewPost()
for _, cm := range channelMembers {
for _, userID := range userIDs {
if permalinkPreviewedPost != nil {
post.Metadata.Embeds[0].Data = permalinkPreviewedPost
}
postForUser := a.sanitizePostMetadataForUserAndChannel(c, post, permalinkPreviewedPost, permalinkPreviewedChannel, cm.UserId)
postForUser := a.sanitizePostMetadataForUserAndChannel(c, post, permalinkPreviewedPost, permalinkPreviewedChannel, userID)
// Using DeepCopy here to avoid a race condition
// between publishing the event and setting the "post" data value below.
messageCopy := message.DeepCopy()
broadcastCopy := messageCopy.GetBroadcast()
broadcastCopy.UserId = cm.UserId
broadcastCopy.UserId = userID
messageCopy.SetBroadcast(broadcastCopy)
postJSON, jsonErr := postForUser.ToJSON()
@ -1260,7 +1260,7 @@ func (a *App) AddCursorIdsForPostList(originalList *model.PostList, afterPost, b
func (a *App) GetPostsForChannelAroundLastUnread(c request.CTX, channelID, userID string, limitBefore, limitAfter int, skipFetchThreads bool, collapsedThreads, collapsedThreadsExtended bool) (*model.PostList, *model.AppError) {
var member *model.ChannelMember
var err *model.AppError
if member, err = a.GetChannelMember(c, channelID, userID); err != nil {
if member, err = a.Srv().getChannelMemberOnly(c, channelID, userID); err != nil {
return nil, err
} else if member.LastViewedAt == 0 {
return model.NewPostList(), nil
@ -1836,9 +1836,9 @@ func (a *App) countThreadMentions(c request.CTX, user *model.User, post *model.P
// countMentionsFromPost returns the number of posts in the post's channel that mention the user after and including the
// given post.
func (a *App) countMentionsFromPost(c request.CTX, user *model.User, post *model.Post) (int, int, int, *model.AppError) {
channel, err := a.GetChannel(c, post.ChannelId)
if err != nil {
return 0, 0, 0, err
channel, appErr := a.GetChannel(c, post.ChannelId)
if appErr != nil {
return 0, 0, 0, appErr
}
if channel.Type == model.ChannelTypeDirect || channel.Type == model.ChannelTypeGroup {
@ -1859,15 +1859,15 @@ func (a *App) countMentionsFromPost(c request.CTX, user *model.User, post *model
return count, countRoot, urgentCount, nil
}
channelMember, err := a.GetChannelMember(c, channel.Id, user.Id)
members, err := a.Srv().Store().Channel().GetAllChannelMembersNotifyPropsForChannel(channel.Id, true)
if err != nil {
return 0, 0, 0, err
return 0, 0, 0, model.NewAppError("countMentionsFromPost", "app.channel.count_posts_since.app_error", nil, "", http.StatusInternalServerError).Wrap(err)
}
keywords := MentionKeywords{}
keywords.AddUser(
user,
channelMember.NotifyProps,
members[user.Id],
&model.Status{Status: model.StatusOnline}, // Assume the user is online since they would've triggered this
true, // Assume channel mentions are always allowed for simplicity
)
@ -1877,9 +1877,9 @@ func (a *App) countMentionsFromPost(c request.CTX, user *model.User, post *model
// A mapping of thread root IDs to whether or not a post in that thread mentions the user
mentionedByThread := make(map[string]bool)
thread, err := a.GetPostThread(post.Id, model.GetPostsOptions{}, user.Id)
if err != nil {
return 0, 0, 0, err
thread, appErr := a.GetPostThread(post.Id, model.GetPostsOptions{}, user.Id)
if appErr != nil {
return 0, 0, 0, appErr
}
count := 0

View file

@ -955,16 +955,16 @@ func (s *OpenTracingLayerChannelStore) GetAll(teamID string) ([]*model.Channel,
return result, err
}
func (s *OpenTracingLayerChannelStore) GetAllChannelMembersById(id string) ([]string, error) {
func (s *OpenTracingLayerChannelStore) GetAllChannelMemberIdsByChannelId(id string) ([]string, error) {
origCtx := s.Root.Store.Context()
span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "ChannelStore.GetAllChannelMembersById")
span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "ChannelStore.GetAllChannelMemberIdsByChannelId")
s.Root.Store.SetContext(newCtx)
defer func() {
s.Root.Store.SetContext(origCtx)
}()
defer span.Finish()
result, err := s.ChannelStore.GetAllChannelMembersById(id)
result, err := s.ChannelStore.GetAllChannelMemberIdsByChannelId(id)
if err != nil {
span.LogFields(spanlog.Error(err))
ext.Error.Set(span, true)
@ -1562,6 +1562,24 @@ func (s *OpenTracingLayerChannelStore) GetMemberForPost(postID string, userID st
return result, err
}
func (s *OpenTracingLayerChannelStore) GetMemberOnly(ctx context.Context, channelID string, userID string) (*model.ChannelMember, error) {
origCtx := s.Root.Store.Context()
span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "ChannelStore.GetMemberOnly")
s.Root.Store.SetContext(newCtx)
defer func() {
s.Root.Store.SetContext(origCtx)
}()
defer span.Finish()
result, err := s.ChannelStore.GetMemberOnly(ctx, channelID, userID)
if err != nil {
span.LogFields(spanlog.Error(err))
ext.Error.Set(span, true)
}
return result, err
}
func (s *OpenTracingLayerChannelStore) GetMembers(channelID string, offset int, limit int) (model.ChannelMembers, error) {
origCtx := s.Root.Store.Context()
span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "ChannelStore.GetMembers")

View file

@ -1039,11 +1039,11 @@ func (s *RetryLayerChannelStore) GetAll(teamID string) ([]*model.Channel, error)
}
func (s *RetryLayerChannelStore) GetAllChannelMembersById(id string) ([]string, error) {
func (s *RetryLayerChannelStore) GetAllChannelMemberIdsByChannelId(id string) ([]string, error) {
tries := 0
for {
result, err := s.ChannelStore.GetAllChannelMembersById(id)
result, err := s.ChannelStore.GetAllChannelMemberIdsByChannelId(id)
if err == nil {
return result, nil
}
@ -1738,6 +1738,27 @@ func (s *RetryLayerChannelStore) GetMemberForPost(postID string, userID string)
}
func (s *RetryLayerChannelStore) GetMemberOnly(ctx context.Context, channelID string, userID string) (*model.ChannelMember, error) {
tries := 0
for {
result, err := s.ChannelStore.GetMemberOnly(ctx, channelID, userID)
if err == nil {
return result, nil
}
if !isRepeatableError(err) {
return result, err
}
tries++
if tries >= 3 {
err = errors.Wrap(err, "giving up after 3 consecutive repeatable transaction failures")
return result, err
}
timepkg.Sleep(100 * timepkg.Millisecond)
}
}
func (s *RetryLayerChannelStore) GetMembers(channelID string, offset int, limit int) (model.ChannelMembers, error) {
tries := 0

View file

@ -39,7 +39,7 @@ func (c *SearchChannelStore) indexChannel(channel *model.Channel) {
var userIDs, teamMemberIDs []string
var err error
if channel.Type == model.ChannelTypePrivate {
userIDs, err = c.GetAllChannelMembersById(channel.Id)
userIDs, err = c.GetAllChannelMemberIdsByChannelId(channel.Id)
if err != nil {
mlog.Warn("Encountered error while indexing channel", mlog.String("channel_id", channel.Id), mlog.Err(err))
return

View file

@ -1125,26 +1125,16 @@ func (s SqlChannelStore) GetChannelsByUser(userId string, includeDeleted bool, l
return channels, nil
}
func (s SqlChannelStore) GetAllChannelMembersById(channelID string) ([]string, error) {
sql, args, err := s.channelMembersForTeamWithSchemeSelectQuery.Where(sq.Eq{
"ChannelId": channelID,
}).ToSql()
if err != nil {
return nil, errors.Wrap(err, "GetAllChannelMembersById_ToSql")
}
dbMembers := channelMemberWithSchemeRolesList{}
err = s.GetReplicaX().Select(&dbMembers, sql, args...)
func (s SqlChannelStore) GetAllChannelMemberIdsByChannelId(channelID string) ([]string, error) {
userIDs := []string{}
err := s.GetReplicaX().Select(&userIDs, `SELECT UserId
FROM ChannelMembers
WHERE ChannelId=?`, channelID)
if err != nil {
return nil, errors.Wrapf(err, "failed to get ChannelMembers with channelID=%s", channelID)
}
res := make([]string, len(dbMembers))
for i, member := range dbMembers.ToModel() {
res[i] = member.UserId
}
return res, nil
return userIDs, nil
}
func (s SqlChannelStore) GetAllChannels(offset, limit int, opts store.ChannelSearchOpts) (model.ChannelListWithTeamData, error) {
@ -2069,6 +2059,33 @@ func (s SqlChannelStore) GetMember(ctx context.Context, channelID string, userID
return dbMember.ToModel(), nil
}
func (s SqlChannelStore) GetMemberOnly(ctx context.Context, channelID string, userID string) (*model.ChannelMember, error) {
var dbMember model.ChannelMember
if err := s.DBXFromContext(ctx).Get(&dbMember, `SELECT ChannelId,
UserId,
Roles,
LastViewedAt,
MsgCount,
MentionCount,
MentionCountRoot,
COALESCE(UrgentMentionCount, 0) AS UrgentMentionCount,
MsgCountRoot,
NotifyProps,
LastUpdateAt,
SchemeUser,
SchemeAdmin,
SchemeGuest
FROM ChannelMembers
WHERE ChannelId=? AND UserId=?`, channelID, userID); err != nil {
if err == sql.ErrNoRows {
return nil, store.NewErrNotFound("ChannelMember", fmt.Sprintf("channelId=%s, userId=%s", channelID, userID))
}
return nil, errors.Wrapf(err, "failed to get ChannelMember with channelId=%s and userId=%s", channelID, userID)
}
return &dbMember, nil
}
func (s SqlChannelStore) InvalidateAllChannelMembersForUser(userId string) {
allChannelMembersForUserCache.Remove(userId)
allChannelMembersForUserCache.Remove(userId + "_deleted")

View file

@ -212,7 +212,7 @@ type ChannelStore interface {
GetDeleted(team_id string, offset int, limit int, userID string) (model.ChannelList, error)
GetChannels(teamID, userID string, opts *model.ChannelSearchOpts) (model.ChannelList, error)
GetChannelsByUser(userID string, includeDeleted bool, lastDeleteAt, pageSize int, fromChannelID string) (model.ChannelList, error)
GetAllChannelMembersById(id string) ([]string, error)
GetAllChannelMemberIdsByChannelId(id string) ([]string, error)
GetAllChannels(page, perPage int, opts ChannelSearchOpts) (model.ChannelListWithTeamData, error)
GetAllChannelsCount(opts ChannelSearchOpts) (int64, error)
GetMoreChannels(teamID string, userID string, offset int, limit int) (model.ChannelList, error)
@ -234,6 +234,9 @@ type ChannelStore interface {
UpdateMemberNotifyProps(channelID, userID string, props map[string]string) (*model.ChannelMember, error)
GetMembers(channelID string, offset, limit int) (model.ChannelMembers, error)
GetMember(ctx context.Context, channelID string, userID string) (*model.ChannelMember, error)
// GetMemberOnly is a lite version of GetMember where it does not join
// with the different schemes tables.
GetMemberOnly(ctx context.Context, channelID string, userID string) (*model.ChannelMember, error)
GetChannelMembersTimezones(channelID string) ([]model.StringMap, error)
GetAllChannelMembersForUser(userID string, allowFromCache bool, includeDeleted bool) (map[string]string, error)
GetChannelsMemberCount(channelIDs []string) (map[string]int64, error)

View file

@ -245,6 +245,10 @@ func testChannelStoreSaveDirectChannel(t *testing.T, ss store.Store, s SqlStore)
require.NoError(t, nErr)
require.Len(t, members, 2, "should have saved 2 members")
userIDs, nErr := ss.Channel().GetAllChannelMemberIdsByChannelId(o1.Id)
require.NoError(t, nErr)
require.ElementsMatch(t, []string{u1.Id, u2.Id}, userIDs)
_, nErr = ss.Channel().SaveDirectChannel(&o1, &m1, &m2)
require.Error(t, nErr, "shouldn't be a able to update from save")
@ -281,6 +285,10 @@ func testChannelStoreSaveDirectChannel(t *testing.T, ss store.Store, s SqlStore)
require.NoError(t, nErr)
require.Len(t, members, 1, "should have saved just 1 member")
userIDs, nErr = ss.Channel().GetAllChannelMemberIdsByChannelId(o1.Id)
require.NoError(t, nErr)
require.ElementsMatch(t, []string{u1.Id}, userIDs)
// Manually truncate Channels table until testlib can handle cleanups
s.GetMasterX().Exec("TRUNCATE Channels")
}
@ -7472,6 +7480,10 @@ func testChannelStoreRemoveAllDeactivatedMembers(t *testing.T, ss store.Store, s
assert.NoError(t, err)
assert.Len(t, d1, 3)
userIDs, nErr := ss.Channel().GetAllChannelMemberIdsByChannelId(c1.Id)
require.NoError(t, nErr)
require.ElementsMatch(t, []string{u1.Id, u2.Id, u3.Id}, userIDs)
// Deactivate users 1 & 2.
u1.DeleteAt = model.GetMillis()
u2.DeleteAt = model.GetMillis()
@ -7489,6 +7501,10 @@ func testChannelStoreRemoveAllDeactivatedMembers(t *testing.T, ss store.Store, s
assert.Len(t, d2, 1)
assert.Equal(t, u3.Id, d2[0].UserId)
userIDs, nErr = ss.Channel().GetAllChannelMemberIdsByChannelId(c1.Id)
require.NoError(t, nErr)
require.ElementsMatch(t, []string{u3.Id}, userIDs)
// Manually truncate Channels table until testlib can handle cleanups
s.GetMasterX().Exec("TRUNCATE Channels")
}

View file

@ -432,8 +432,8 @@ func (_m *ChannelStore) GetAll(teamID string) ([]*model.Channel, error) {
return r0, r1
}
// GetAllChannelMembersById provides a mock function with given fields: id
func (_m *ChannelStore) GetAllChannelMembersById(id string) ([]string, error) {
// GetAllChannelMemberIdsByChannelId provides a mock function with given fields: id
func (_m *ChannelStore) GetAllChannelMemberIdsByChannelId(id string) ([]string, error) {
ret := _m.Called(id)
var r0 []string
@ -1314,6 +1314,32 @@ func (_m *ChannelStore) GetMemberForPost(postID string, userID string) (*model.C
return r0, r1
}
// GetMemberOnly provides a mock function with given fields: ctx, channelID, userID
func (_m *ChannelStore) GetMemberOnly(ctx context.Context, channelID string, userID string) (*model.ChannelMember, error) {
ret := _m.Called(ctx, channelID, userID)
var r0 *model.ChannelMember
var r1 error
if rf, ok := ret.Get(0).(func(context.Context, string, string) (*model.ChannelMember, error)); ok {
return rf(ctx, channelID, userID)
}
if rf, ok := ret.Get(0).(func(context.Context, string, string) *model.ChannelMember); ok {
r0 = rf(ctx, channelID, userID)
} else {
if ret.Get(0) != nil {
r0 = ret.Get(0).(*model.ChannelMember)
}
}
if rf, ok := ret.Get(1).(func(context.Context, string, string) error); ok {
r1 = rf(ctx, channelID, userID)
} else {
r1 = ret.Error(1)
}
return r0, r1
}
// GetMembers provides a mock function with given fields: channelID, offset, limit
func (_m *ChannelStore) GetMembers(channelID string, offset int, limit int) (model.ChannelMembers, error) {
ret := _m.Called(channelID, offset, limit)

View file

@ -907,10 +907,10 @@ func (s *TimerLayerChannelStore) GetAll(teamID string) ([]*model.Channel, error)
return result, err
}
func (s *TimerLayerChannelStore) GetAllChannelMembersById(id string) ([]string, error) {
func (s *TimerLayerChannelStore) GetAllChannelMemberIdsByChannelId(id string) ([]string, error) {
start := time.Now()
result, err := s.ChannelStore.GetAllChannelMembersById(id)
result, err := s.ChannelStore.GetAllChannelMemberIdsByChannelId(id)
elapsed := float64(time.Since(start)) / float64(time.Second)
if s.Root.Metrics != nil {
@ -918,7 +918,7 @@ func (s *TimerLayerChannelStore) GetAllChannelMembersById(id string) ([]string,
if err == nil {
success = "true"
}
s.Root.Metrics.ObserveStoreMethodDuration("ChannelStore.GetAllChannelMembersById", success, elapsed)
s.Root.Metrics.ObserveStoreMethodDuration("ChannelStore.GetAllChannelMemberIdsByChannelId", success, elapsed)
}
return result, err
}
@ -1451,6 +1451,22 @@ func (s *TimerLayerChannelStore) GetMemberForPost(postID string, userID string)
return result, err
}
func (s *TimerLayerChannelStore) GetMemberOnly(ctx context.Context, channelID string, userID string) (*model.ChannelMember, error) {
start := time.Now()
result, err := s.ChannelStore.GetMemberOnly(ctx, channelID, userID)
elapsed := float64(time.Since(start)) / float64(time.Second)
if s.Root.Metrics != nil {
success := "false"
if err == nil {
success = "true"
}
s.Root.Metrics.ObserveStoreMethodDuration("ChannelStore.GetMemberOnly", success, elapsed)
}
return result, err
}
func (s *TimerLayerChannelStore) GetMembers(channelID string, offset int, limit int) (model.ChannelMembers, error) {
start := time.Now()

View file

@ -526,7 +526,7 @@ func (worker *BleveIndexerWorker) BulkIndexChannels(logger mlog.LoggerIFace, cha
var userIDs []string
var err error
if channel.Type == model.ChannelTypePrivate {
userIDs, err = worker.jobServer.Store.Channel().GetAllChannelMembersById(channel.Id)
userIDs, err = worker.jobServer.Store.Channel().GetAllChannelMemberIdsByChannelId(channel.Id)
if err != nil {
return nil, model.NewAppError("BleveIndexerWorker.BulkIndexChannels", "bleveengine.indexer.do_job.bulk_index_channels.batch_error", nil, "", http.StatusInternalServerError).Wrap(err)
}