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

improve possible performance bottleneck #28547

Merged
merged 4 commits into from
Dec 21, 2023
Merged

improve possible performance bottleneck #28547

merged 4 commits into from
Dec 21, 2023

Conversation

lunny
Copy link
Member

@lunny lunny commented Dec 20, 2023

Replace #28500

@lunny lunny requested a review from delvh December 20, 2023 05:18
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 20, 2023
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 20, 2023
@lunny lunny added performance/speed performance issues with slow downs backport/v1.21 This PR should be backported to Gitea 1.21 labels Dec 20, 2023
@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 Dec 20, 2023
@6543 6543 added this to the 1.22.0 milestone Dec 20, 2023
@KN4CK3R
Copy link
Member

KN4CK3R commented Dec 21, 2023

Can't you just join the repository table to that query and get rid of the iteration?

@pull-request-size pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 21, 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 Dec 21, 2023
@KN4CK3R KN4CK3R added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Dec 21, 2023
@6543 6543 merged commit b35d3fd into go-gitea:main Dec 21, 2023
25 checks passed
@GiteaBot GiteaBot mentioned this pull request Dec 21, 2023
Merged
GiteaBot added a commit to GiteaBot/gitea that referenced this pull request Dec 21, 2023
@lunny @GiteaBot
improve possible performance bottleneck (go-gitea#28547)
14c3cbf 
Replace go-gitea#28500

---------

Co-authored-by: Giteabot <[email protected]>
@GiteaBot GiteaBot added backport/done All backports for this PR have been created and removed reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. labels Dec 21, 2023
@lunny lunny deleted the lunny/avoid_update_where_in branch December 21, 2023 22:45
lunny added a commit that referenced this pull request Dec 21, 2023
@GiteaBot @lunny
improve possible performance bottleneck (#28547) (#28578)
1a3803e 
Backport #28547 by @lunny

Replace #28500

Co-authored-by: Lunny Xiao <[email protected]>
zjjhot added a commit to zjjhot/gitea that referenced this pull request Dec 22, 2023
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
9296ce0 
* giteaofficial/main:
  Add more ways to try (go-gitea#28581)
  Convert to url auth to header auth in tests (go-gitea#28484)
  Fix 500 error of searching commits (go-gitea#28576)
  improve possible performance bottleneck (go-gitea#28547)
  Use information from previous blame parts (go-gitea#28572)
  Make offline mode as default to no connect external avatar service by default (go-gitea#28548)
  Fix merging artifact chunks error when minio storage basepath is set (go-gitea#28555)
  feat: bump `dessant/lock-threads` and `actions/setup-go` to use nodejs20 runtime (go-gitea#28565)
  Update actions document about comparsion as Github Actions (go-gitea#28560)
  Fix inperformant query on retrifing review from database. (go-gitea#28552)
  Fix the issue ref rendering for wiki (go-gitea#28556)
  Add missing head of lfs client batch (go-gitea#28550)
  [skip ci] Updated translations via Crowdin
  Remove deadcode under models/issues (go-gitea#28536)
  Always enable caches (go-gitea#28527)
  Improve ObjectFormat interface (go-gitea#28496)
@fnetX
Copy link
Contributor

fnetX commented Dec 22, 2023

Just adding here a quote from the Forgejo fix at #28500

which is not possible in a UPDATE query for SQLite

We noticed that you apparently did not make a special case for SQLite, which likely breaks support for this database as per this PR.Test coverage for this part is not enough, thus not detected, I assume. Maybe I'm completely blind, but can you explain how the PR is supposed to work with SQLite?

@lunny
Copy link
Member Author

lunny commented Dec 22, 2023 •
edited
Loading

Acorrding to the https://www.sqlite.org/lang_update.html, sqlite supports Update Join. But I also sent a PR #28590

@lunny lunny mentioned this pull request Dec 22, 2023
Merged
@fnetX
Copy link
Contributor

fnetX commented Dec 22, 2023

Okay, maybe I confused it with DELETE and JOIN, sorry for bothering in this case. Did you check whether it works or not, so we don't attempt to fix bugs that don't actually exist? There is no automated testing for this one, and creating a test case with migrations wasn't trivial. I did not yet spend time on it.

@lunny
Copy link
Member Author

lunny commented Dec 23, 2023

Okay, maybe I confused it with DELETE and JOIN, sorry for bothering in this case. Did you check whether it works or not, so we don't attempt to fix bugs that don't actually exist? There is no automated testing for this one, and creating a test case with migrations wasn't trivial. I did not yet spend time on it.

Yes, I will send a PR with tests for it.

techknowlogick pushed a commit to techknowlogick/gitea that referenced this pull request Dec 23, 2023
@lunny @GiteaBot @techknowlogick
improve possible performance bottleneck (go-gitea#28547)
041436d 
Replace go-gitea#28500

---------

Co-authored-by: Giteabot <[email protected]>
lunny added a commit to lunny/gitea that referenced this pull request Dec 23, 2023
@lunny
Revert "improve possible performance bottleneck (go-gitea#28547)"
a8051ed 
This reverts commit b35d3fd.
@lunny
Copy link
Member Author

lunny commented Dec 23, 2023

I will send a revert first and then try to send a new PR. #28593

lunny added a commit that referenced this pull request Dec 25, 2023
@lunny
Revert "improve possible performance bottleneck (#28547)" (#28593)
0407a40 
This reverts commit b35d3fd.

This is totally wrong. I think `Update join` hasn't been supported well
by xorm.

I just revert the PR and will try to send another one.
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Dec 25, 2023
@lunny @GiteaBot
Revert "improve possible performance bottleneck (go-gitea#28547)" (go…
f4c526e 
…-gitea#28593)

This reverts commit b35d3fd.

This is totally wrong. I think `Update join` hasn't been supported well
by xorm.

I just revert the PR and will try to send another one.
zjjhot added a commit to zjjhot/gitea that referenced this pull request Dec 25, 2023
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
a548fe7 
* giteaofficial/main:
  Added instance-level variables (go-gitea#28115)
  Revert "improve possible performance bottleneck (go-gitea#28547)" (go-gitea#28593)
  [skip ci] Updated licenses and gitignores
  Fix flex container width (go-gitea#28603)
  Fix the scroll behavior for emoji/mention list (go-gitea#28597)
  bump to use alpine3.19 (go-gitea#28594)
  Include heap pprof in diagnosis report to help debugging memory leaks (go-gitea#28596)
  Disable query token param in integration tests (go-gitea#28592)
  Fix wrong due date rendering in issue list page (go-gitea#28588)
  Fix `status_check_contexts` matching bug (go-gitea#28582)
  Fix 405 method not allowed CORS / OIDC (go-gitea#28583)
lunny added a commit that referenced this pull request Dec 25, 2023
@GiteaBot @lunny
Revert "improve possible performance bottleneck (#28547)" (#28593) (#…
b258833 
…28608)

Backport #28593 by @lunny

This reverts commit b35d3fd.

This is totally wrong. I think `Update join` hasn't been supported well
by xorm.

I just revert the PR and will try to send another one.

Co-authored-by: Lunny Xiao <[email protected]>
@lunny lunny removed this from the 1.22.0 milestone Dec 30, 2023
lunny added a commit that referenced this pull request Dec 31, 2023
@lunny
Upgrade xorm to new version which supported update join for all suppo…
da58bb8 
…rted databases (#28590)

Fix #28547 (comment)

Since https://gitea.com/xorm/xorm/pulls/2383 merged, xorm now supports
UPDATE JOIN.
To keep consistent from different databases, xorm use
`engine.Join().Update`, but the actural generated SQL are different
between different databases.

For MySQL, it's `UPDATE talbe1 JOIN table2 ON join_conditions SET xxx
Where xxx`.

For MSSQL, it's `UPDATE table1 SET xxx FROM TABLE1, TABLE2 WHERE
join_conditions`.

For SQLITE per https://www.sqlite.org/lang_update.html, sqlite support
`UPDATE table1 SET xxx FROM table2 WHERE join conditions` from
3.33.0(2020-8-14).

POSTGRES is the same as SQLITE.
@GiteaBot GiteaBot mentioned this pull request Dec 31, 2023
Merged
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Dec 31, 2023
@lunny @GiteaBot
Upgrade xorm to new version which supported update join for all suppo…
490a250 
…rted databases (go-gitea#28590)

Fix go-gitea#28547 (comment)

Since https://gitea.com/xorm/xorm/pulls/2383 merged, xorm now supports
UPDATE JOIN.
To keep consistent from different databases, xorm use
`engine.Join().Update`, but the actural generated SQL are different
between different databases.

For MySQL, it's `UPDATE talbe1 JOIN table2 ON join_conditions SET xxx
Where xxx`.

For MSSQL, it's `UPDATE table1 SET xxx FROM TABLE1, TABLE2 WHERE
join_conditions`.

For SQLITE per https://www.sqlite.org/lang_update.html, sqlite support
`UPDATE table1 SET xxx FROM table2 WHERE join conditions` from
3.33.0(2020-8-14).

POSTGRES is the same as SQLITE.
lunny added a commit that referenced this pull request Dec 31, 2023
@GiteaBot @lunny
Upgrade xorm to new version which supported update join for all suppo…
18da3f8 
…rted databases (#28590) (#28668)

Backport #28590 by @lunny

Fix #28547 (comment)

Since https://gitea.com/xorm/xorm/pulls/2383 merged, xorm now supports
UPDATE JOIN.
To keep consistent from different databases, xorm use
`engine.Join().Update`, but the actural generated SQL are different
between different databases.

For MySQL, it's `UPDATE talbe1 JOIN table2 ON join_conditions SET xxx
Where xxx`.

For MSSQL, it's `UPDATE table1 SET xxx FROM TABLE1, TABLE2 WHERE
join_conditions`.

For SQLITE per https://www.sqlite.org/lang_update.html, sqlite support
`UPDATE table1 SET xxx FROM table2 WHERE join conditions` from
3.33.0(2020-8-14).

POSTGRES is the same as SQLITE.

Co-authored-by: Lunny Xiao <[email protected]>
DennisRasey pushed a commit to DennisRasey/forgejo that referenced this pull request Jan 16, 2024
@GiteaBot @earl-warren
Upgrade xorm to new version which supported update join for all suppo…
0a157ed 
…rted databases (#28590) (#28668)

Backport #28590 by @lunny

Fix go-gitea/gitea#28547 (comment)

Since https://gitea.com/xorm/xorm/pulls/2383 merged, xorm now supports
UPDATE JOIN.
To keep consistent from different databases, xorm use
`engine.Join().Update`, but the actural generated SQL are different
between different databases.

For MySQL, it's `UPDATE talbe1 JOIN table2 ON join_conditions SET xxx
Where xxx`.

For MSSQL, it's `UPDATE table1 SET xxx FROM TABLE1, TABLE2 WHERE
join_conditions`.

For SQLITE per https://www.sqlite.org/lang_update.html, sqlite support
`UPDATE table1 SET xxx FROM table2 WHERE join conditions` from
3.33.0(2020-8-14).

POSTGRES is the same as SQLITE.

Co-authored-by: Lunny Xiao <[email protected]>
(cherry picked from commit 18da3f8)
fuxiaohei pushed a commit to fuxiaohei/gitea that referenced this pull request Jan 17, 2024
@lunny @GiteaBot @fuxiaohei
improve possible performance bottleneck (go-gitea#28547)
c26ff80 
Replace go-gitea#28500

---------

Co-authored-by: Giteabot <[email protected]>
fuxiaohei pushed a commit to fuxiaohei/gitea that referenced this pull request Jan 17, 2024
@lunny @fuxiaohei
Revert "improve possible performance bottleneck (go-gitea#28547)" (go…
bda7534 
…-gitea#28593)

This reverts commit b35d3fd.

This is totally wrong. I think `Update join` hasn't been supported well
by xorm.

I just revert the PR and will try to send another one.
fuxiaohei pushed a commit to fuxiaohei/gitea that referenced this pull request Jan 17, 2024
@lunny @fuxiaohei
Upgrade xorm to new version which supported update join for all suppo…
a2bd0c9 
…rted databases (go-gitea#28590)

Fix go-gitea#28547 (comment)

Since https://gitea.com/xorm/xorm/pulls/2383 merged, xorm now supports
UPDATE JOIN.
To keep consistent from different databases, xorm use
`engine.Join().Update`, but the actural generated SQL are different
between different databases.

For MySQL, it's `UPDATE talbe1 JOIN table2 ON join_conditions SET xxx
Where xxx`.

For MSSQL, it's `UPDATE table1 SET xxx FROM TABLE1, TABLE2 WHERE
join_conditions`.

For SQLITE per https://www.sqlite.org/lang_update.html, sqlite support
`UPDATE table1 SET xxx FROM table2 WHERE join conditions` from
3.33.0(2020-8-14).

POSTGRES is the same as SQLITE.
silverwind pushed a commit to silverwind/gitea that referenced this pull request Feb 20, 2024
@lunny @GiteaBot @silverwind
improve possible performance bottleneck (go-gitea#28547)
1208676 
Replace go-gitea#28500

---------

Co-authored-by: Giteabot <[email protected]>
silverwind pushed a commit to silverwind/gitea that referenced this pull request Feb 20, 2024
@lunny @silverwind
Revert "improve possible performance bottleneck (go-gitea#28547)" (go…
4b92a56 
…-gitea#28593)

This reverts commit b35d3fd.

This is totally wrong. I think `Update join` hasn't been supported well
by xorm.

I just revert the PR and will try to send another one.
silverwind pushed a commit to silverwind/gitea that referenced this pull request Feb 20, 2024
@lunny @silverwind
Upgrade xorm to new version which supported update join for all suppo…
6225614 
…rted databases (go-gitea#28590)

Fix go-gitea#28547 (comment)

Since https://gitea.com/xorm/xorm/pulls/2383 merged, xorm now supports
UPDATE JOIN.
To keep consistent from different databases, xorm use
`engine.Join().Update`, but the actural generated SQL are different
between different databases.

For MySQL, it's `UPDATE talbe1 JOIN table2 ON join_conditions SET xxx
Where xxx`.

For MSSQL, it's `UPDATE table1 SET xxx FROM TABLE1, TABLE2 WHERE
join_conditions`.

For SQLITE per https://www.sqlite.org/lang_update.html, sqlite support
`UPDATE table1 SET xxx FROM table2 WHERE join conditions` from
3.33.0(2020-8-14).

POSTGRES is the same as SQLITE.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@6543 6543 6543 approved these changes

@KN4CK3R KN4CK3R KN4CK3R approved these changes

@delvh delvh Awaiting requested review from delvh

Assignees
No one assigned
Labels
backport/done All backports for this PR have been created backport/v1.21 This PR should be backported to Gitea 1.21 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. performance/speed performance issues with slow downs size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lunny @KN4CK3R @fnetX @6543 @GiteaBot