Skip to content
This repository was archived by the owner on May 5, 2025. It is now read-only.

Conversation

@Swatinem
Copy link
Contributor

There are a few very slow DELETE queries as part of repo deletion. The worst of those is deleting the newly created "shadow owner", which takes >1 minute.

Running all the on_delete hooks using django (which has since been fixes) has revealed that the SET NULL of Commit.author is the culprit, as that column did not have any index previously. That is similar to the SET NULL on Repository.fork, except that query is by far not as slow, probably because of a lot less records that need to be scanned.


While at it, this also fixes a django warning related to the default not being a callable, and aligns the Repository.fork SET NULL constraint with the reality of the database.


The generated SQL for the migration looks like this. Interestingly, altering the fields does not run any actual migration here, as it seems it only has an effect on the django migration state.

--
-- Alter field fork on repository
--
-- (no-op)
--
-- Alter field languages on repository
--
-- (no-op)
--
-- Concurrently create index commits_author_cd2f50_idx on field(s) author of model commit
--
CREATE INDEX CONCURRENTLY "commits_author_cd2f50_idx" ON "commits" ("author");
--
-- Concurrently create index repos_forkid_4cd440_idx on field(s) fork of model repository
--
CREATE INDEX CONCURRENTLY "repos_forkid_4cd440_idx" ON "repos" ("forkid");

There are a few very slow `DELETE` queries as part of repo deletion.
The worst of those is deleting the newly created "shadow owner", which takes >1 minute.

Running all the `on_delete` hooks using django (which has since been fixes) has revealed that the `SET NULL` of `Commit.author` is the culprit, as that column did not have any index previously.
That is similar to the `SET NULL` on `Repository.fork`, except that query is by far not as slow, probably because of a lot less records that need to be scanned.

---

While at it, this also fixes a django warning related to the `default` not being a callable, and aligns the `Repository.fork` `SET NULL` constraint with the reality of the database.
@Swatinem Swatinem requested a review from a team January 27, 2025 13:34
@Swatinem Swatinem self-assigned this Jan 27, 2025
@Swatinem Swatinem added this pull request to the merge queue Jan 29, 2025
Merged via the queue into main with commit 32938c0 Jan 29, 2025
6 checks passed
@Swatinem Swatinem deleted the swatinem/deletion-indices branch January 29, 2025 08:10
@codecov
Copy link

codecov bot commented Jan 29, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.90%. Comparing base (8db93ad) to head (421097a).
Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #490      +/-   ##
==========================================
- Coverage   90.32%   89.90%   -0.42%     
==========================================
  Files         435      324     -111     
  Lines       12999     9114    -3885     
  Branches     2112     1615     -497     
==========================================
- Hits        11741     8194    -3547     
+ Misses       1143      857     -286     
+ Partials      115       63      -52     
Flag Coverage Δ
shared-docker-uploader ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants