Skip to content

Improve action table indices #19472

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 16 commits into from
Jun 18, 2022
Merged

Improve action table indices #19472

merged 16 commits into from
Jun 18, 2022

Conversation

zeripath
Copy link
Contributor

Improve the indices on the action table by creating a covering index that covers the
common queries and removes unhelpful indices.

Fix #16665

Signed-off-by: Andrew Thornton art27@cantab.net

@zeripath
Improve action table indices
133c63e 
Improve the indices on the action table by creating a covering index that covers the
common queries and removes unhelpful indices.

Fix go-gitea#16665

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath added type/enhancement An improvement of existing functionality performance/speed performance issues with slow downs labels Apr 23, 2022
@zeripath zeripath added this to the 1.17.0 milestone Apr 23, 2022
@zeripath
Copy link
Contributor Author

Ideally we might want to wait till some functionality that would allow us to assert the ordering of the index entries eg. https://gitea.com/xorm/xorm/pulls/2137 is merged and xorm is updated

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 23, 2022
delvh
delvh reviewed May 9, 2022
View reviewed changes
@lunny
Copy link
Member

lunny commented Jun 4, 2022

Is there any scene to use user_id as an index? On user's profile and activies page?

delvh
delvh approved these changes Jun 4, 2022
View reviewed changes
Copy link
Member

@delvh delvh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lunny If I see that correctly, notifications?

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jun 4, 2022
@lunny
Copy link
Member

lunny commented Jun 4, 2022

@lunny If I see that correctly, notifications?

So the first column of a composite index will become a standalone index column?

@delvh
Copy link
Member

delvh commented Jun 4, 2022

If I had to guess how the database query optimizers are built, I'd say so.
But perhaps @wxiaoguang can clarify it for us.

@zeripath @delvh
Update models/migrations/migrations.go
a1f182b 
Co-authored-by: delvh <dev.lh@web.de>
@zeripath
Copy link
Contributor Author

zeripath commented Jun 4, 2022 •
edited
Loading

So the first column of a composite index will become a standalone index column?

Yes an index on (A, B, C, D) can provide indexes for (A), (A,B), (A,B,C) and (A,B,C,D).

Is there any scene to use user_id as an index? On user's profile and activies page?

If instead of having, INDEX(u_ua_and_r) INDEX(ua_and_r) INDEX(r) we could reorder things so we could just have one or two indices.

See #16665 (comment):

  1. Drop the is_deleted, is_private and created_unix indices - they are useless
  2. At the minimum we should add:
  • act_user_id repo_id user_id (folded into one of the below)
  • repo_id user_id
  • act_user_id repo_id
  1. Consider adding in:
  • act_user_id repo_id user_id is_deleted
  1. We could consider adding further composites with is_deleted added in to the above but it will depend on how frequent is_deleted and I think we would need to actually do some live testing on this.
  2. We always sort by created_unix which is a good reason to include the created_unix in these proposed indices so always add these in somewhere

One we have the OrderBy fix in we can change these indices to be:

  1. ( act_user_id, repo_id, created_unix, user_id) (consider including is_deleted here too)
  2. ( repo_id, created_unix, user_id)

But we'd need to check again on MySQL to ensure the position of created_unix is correct. It'd be helpful to check on postgres too.

@lunny
Copy link
Member

lunny commented Jun 4, 2022

So the first column of a composite index will become a standalone index column?

Yes an index on (A, B, C, D) can provide indexes for (A), (A,B), (A,B,C) and (A,B,C,D).

Is there any scene to use user_id as an index? On user's profile and activies page?

If instead of having, INDEX(u_ua_and_r) INDEX(ua_and_r) INDEX(r) we could reorder things so we could just have one or two indices.

See #16665 (comment):

  1. Drop the is_deleted, is_private and created_unix indices - they are useless
  2. At the minimum we should add:
  • act_user_id repo_id user_id (folded into one of the below)
  • repo_id user_id
  • act_user_id repo_id
  1. Consider adding in:
  • act_user_id repo_id user_id is_deleted
  1. We could consider adding further composites with is_deleted added in to the above but it will depend on how frequent is_deleted and I think we would need to actually do some live testing on this.
  2. We always sort by created_unix which is a good reason to include the created_unix in these proposed indices so always add these in somewhere

One we have the OrderBy fix in we can change these indices to be:

  1. ( act_user_id, repo_id, created_unix, user_id) (consider including is_deleted here too)
  2. ( repo_id, created_unix, user_id)

But we'd need to check again on MySQL to ensure the position of created_unix is correct. It'd be helpful to check on postgres too.

Wow, so let's wait #19849

@lunny
Copy link
Member

lunny commented Jun 16, 2022

Since #19874 merged, please resolve the conflicts.

@lunny lunny modified the milestones: 1.17.0, 1.18.0 Jun 16, 2022
@zeripath zeripath removed this from the 1.18.0 milestone Jun 17, 2022
@zeripath zeripath added this to the 1.17.0 milestone Jun 17, 2022
@zeripath
Copy link
Contributor Author

I think we should try to get this in to 1.17

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jun 17, 2022
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jun 17, 2022 •
edited
Loading

LGTM since it was scheduled in 1.17 before frozen

lunny
lunny reviewed Jun 17, 2022
View reviewed changes
models/migrations/v216.go
"xorm.io/xorm/schemas"
)

type improveActionTableIndicesAction struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know whether a private struct could work with xorm.

Copy link
Contributor Author

@zeripath zeripath Jun 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if you're meaning this as a comment, e.g. I didn't know that a private struct would work with xorm, or if you're trying to express doubt that this will work, e.g. I don't think a private struct will work with xorm.

It will work.

If you are given a pointer to a nonExportedStruct:

package someplace

type notExportedStruct {
   Exported string
}

func NewNotExported() *notExportedStruct {
  return &notExportedStruct{}
}

you can always create another copy of it using:

package main

import "someplace"

func main() {
  notExported := somplace.NewNotExported()

	val := reflect.ValueOf(notExported).Elem()
	typ := val.Type()

	newNotExportedVal := reflect.New(typ)
  newNotExportedVal.Elem().FieldByName("Exported").SetString("I have been set by reflection")

	fmt.Println(newNotExportedVal.Interface())
}

Which is what xorm does internally anyway.

Xorm doesn't need to know or be able to cast to the non-exported type - it just uses reflection to look across the exported fields which will always work or casts to interfaces it knows e.g. TableName. newNotExportedVal can be cast to an interface{} using the .Interface() function as normal.

lunny
lunny reviewed Jun 17, 2022
View reviewed changes
@techknowlogick techknowlogick merged commit 5d653cc into go-gitea:main Jun 18, 2022
@zeripath zeripath deleted the action_indices branch June 19, 2022 14:01
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jun 20, 2022
@zjjhot
Merge remote-tracking branch 'giteaofficial/release/v1.17'
d03cd54 
* giteaofficial/release/v1.17: (35 commits)
  Simplify and fix migration 216 (go-gitea#20036)
  Alter hook_task TEXT fields to LONGTEXT (go-gitea#20038) (go-gitea#20041)
  Backtick table name in generic orphan check (go-gitea#20019) (go-gitea#20037)
  Respond with a 401 on git push when password isn't changed yet (go-gitea#20027)
  Fix delete pull head ref for DeleteIssue (go-gitea#20032)  (go-gitea#20034)
  use quoted regexp instead of git fixed-value (go-gitea#20030)
  Dump should only copy regular files and symlink regular files (go-gitea#20015) (go-gitea#20021)
  Return 404 when tag is broken (go-gitea#20024)
  [skip ci] Updated translations via Crowdin
  [skip ci] Updated translations via Crowdin
  Add fgprof pprof profiler (go-gitea#20005)
  [skip ci] Updated translations via Crowdin
  Improve action table indices (go-gitea#19472)
  Add dbconsistency checks for Stopwatches (go-gitea#20010)
  fix push mirrors URL are no longer displayed on the UI (go-gitea#20011)
  Empty log queue on flush and close (go-gitea#19994)
  [skip ci] Updated translations via Crowdin
  Stop spurious APIFormat stopwatches logs (go-gitea#20008)
  Fix CountOrphanedLabels in orphan check (go-gitea#20009)
  Write Commit-Graphs in RepositoryDumper (go-gitea#20004)
  ...
vsysoev pushed a commit to IntegraSDL/gitea that referenced this pull request Aug 10, 2022
@zeripath
Improve action table indices (go-gitea#19472)
2b56301
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 24, 2022
@zeripath @AbdulrhmnGhanem
Improve action table indices (go-gitea#19472)
de0b815
@lunny lunny mentioned this pull request Feb 24, 2023
Closed
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lunny lunny lunny left review comments

@wxiaoguang wxiaoguang wxiaoguang approved these changes

@delvh delvh delvh approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. performance/speed performance issues with slow downs type/enhancement An improvement of existing functionality
Projects
None yet
Milestone
1.17.0
Development

Successfully merging this pull request may close these issues.

Wrongly used index slows down queries on action table (dashboard view)
6 participants
@zeripath @lunny @delvh @wxiaoguang @techknowlogick @GiteaBot