Skip to content
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

Add slow SQL query warning #27545

Merged
merged 8 commits into from
Feb 23, 2024
Merged

Add slow SQL query warning #27545

merged 8 commits into from
Feb 23, 2024

Conversation

earl-warren
Copy link
Contributor

  • Databases are one of the most important parts of Forgejo, every interaction uses the database in one way or another. Therefore, it is important to maintain the database and recognize when the server is not doing well with the database. There already is the option to log every SQL query along with its execution time, but monitoring becomes impractical for larger instances and takes up unnecessary storage in the logs.
  • Add a QoL enhancement that allows instance administrators to specify a threshold value beyond which query execution time is logged as a warning in the xorm logger. The default value is a conservative five seconds to avoid this becoming a source of spam in the logs.
  • The use case for this patch is that with an instance the size of Codeberg, monitoring SQL logs is not very fruitful and most of them are uninteresting. Recently, in the context of persistent deadlock issues (https://codeberg.org/forgejo/forgejo/issues/220), I have noticed that certain queries hold locks on tables like comment and issue for several seconds. This patch helps to identify which queries these are and when they happen.
  • Added unit test.

(cherry picked from commit 9cf501f1af4cd870221cef6af489618785b71186)

@Gusted @earl-warren
add slow SQL query warning
f7b632c 
- Databases are one of the most important parts of Forgejo, every
interaction uses the database in one way or another.
Therefore, it is important to maintain the database and recognize when
the server is not doing well with the database. There already is the
option to log *every* SQL query along with its execution time, but
monitoring becomes impractical for larger instances and takes up
unnecessary storage in the logs.
- Add a QoL enhancement that allows instance administrators to specify a
threshold value beyond which query execution time is logged as a warning
in the xorm logger. The default value is a conservative five seconds to
avoid this becoming a source of spam in the logs.
- The use case for this patch is that with an instance the size of Codeberg, monitoring SQL logs is not very fruitful and most of them are uninteresting. Recently, in the context of persistent deadlock issues (https://codeberg.org/forgejo/forgejo/issues/220), I have noticed that certain queries hold locks on tables like comment and issue for several seconds. This patch helps to identify which queries these are and when they happen.
- Added unit test.

(cherry picked from commit 9cf501f1af4cd870221cef6af489618785b71186)
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 9, 2023
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 9, 2023
@JakobDev
Copy link
Contributor

JakobDev commented Oct 9, 2023

The new Setting needs to be documented

wxiaoguang
wxiaoguang reviewed Oct 9, 2023
View reviewed changes
models/db/engine_test.go Outdated Show resolved Hide resolved
delvh
delvh reviewed Oct 15, 2023
View reviewed changes
models/db/engine.go Show resolved Hide resolved
models/db/engine_test.go Outdated Show resolved Hide resolved
@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 Oct 16, 2023
@delvh delvh added the type/enhancement An improvement of existing functionality label Oct 16, 2023
@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 Jan 21, 2024
@denyskon denyskon added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jan 21, 2024
@denyskon denyskon added this to the 1.22.0 milestone Jan 21, 2024
@wxiaoguang wxiaoguang removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jan 21, 2024
@wxiaoguang
Copy link
Contributor

More thoughts about this feature:

  1. Is it useful? Somewhat, but not really. DBA should already have the ability to find the slow queries from database log.
  2. Is it complete? I think no, because there are also many performance problems are caused "plenty SQLs in a loop", say, even if a single SQL is fast, if it is executed in a loop, the whole request will slow down. But this PR isn't able to catch this case.

So I think it is fine to merge, although it is far from ideal.

ps: my previous about the "tests", I never meant that it doesn't need tests, but actually that test code was wrong, so I think the tests should be rewritten correctly.

6543
6543 approved these changes Feb 23, 2024
View reviewed changes
Copy link
Member

@6543 6543 left a comment

Choose a reason for hiding this comment

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

i think it's a usefull tool ...
... yes it does not cover the whole picture, but if it catches at lest some, its better than nothing

@6543 6543 changed the title add slow SQL query warning Add slow SQL query warning Feb 23, 2024
@6543 6543 added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 23, 2024
@6543 6543 enabled auto-merge (squash) February 23, 2024 00:33
@6543 6543 merged commit b748d62 into go-gitea:main Feb 23, 2024
26 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 23, 2024
zjjhot added a commit to zjjhot/gitea that referenced this pull request Feb 23, 2024
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
d1e6961 
* giteaofficial/main:
  Start to migrate from `util.OptionalBool` to `optional.Option[bool]` (go-gitea#29329)
  Add slow SQL query warning (go-gitea#27545)
  Unify organizations header (go-gitea#29248)
  Frontport changelogs of minor releases (go-gitea#29337)
  Support SAML authentication (go-gitea#25165)
  Upgrade to fabric 6 (go-gitea#29334)
  Don't show third-party JS errors in production builds (go-gitea#29303)
  Remove bountysource (go-gitea#29330)
  Remove unnecessary "Str2html" modifier from templates (go-gitea#29319)
  Ignore the linux anchor point to avoid linux migrate failure (go-gitea#29295)
  Remove jQuery from the repo commit functions (go-gitea#29230)
  Remove unnecessary "Safe" modifier from templates (go-gitea#29318)
  Remove jQuery from the image pasting functionality (go-gitea#29324)
  Improve the `issue_comment` workflow trigger event (go-gitea#29277)
  Properly migrate automatic merge GitLab comments (go-gitea#27873)
  Refactor cmd setup and remove deadcode (go-gitea#29313)
  small cache when get user id on interation (go-gitea#29296)
  Discard unread data of `git cat-file` (go-gitea#29297)
  Don't install playwright twice (go-gitea#29302)

# Conflicts:
#	templates/home.tmpl
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wxiaoguang wxiaoguang wxiaoguang left review comments

@6543 6543 6543 approved these changes

@denyskon denyskon denyskon 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. modifies/docs size/M Denotes a PR that changes 30-99 lines, ignoring generated files. type/enhancement An improvement of existing functionality
Projects
None yet
Milestone
1.22.0
Development

Successfully merging this pull request may close these issues.
8 participants
@earl-warren @JakobDev @wxiaoguang @6543 @denyskon @delvh @GiteaBot @Gusted