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.

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

fix: Remove slow update repos query from trigger function #207

Merged
merged 2 commits into from
Oct 25, 2023

Conversation

scott-codecov
Copy link
Contributor

@scott-codecov scott-codecov commented Oct 24, 2023

Purpose/Motivation

update repos set updatestamp = now() where repoid = ?;

This query is often very slow. I don't know why exactly b/c the condition is only on the primary key. I suspect there's just so much contention for that repo row that we're waiting on locks most of the time. This seems to mostly be the case for repos that receive a lot of continuous uploads.

We recently removed this query from the upload endpoint in #200 and moved it into the worker in codecov/worker#150

Unfortunately it's still running as part of the insert commit query though due to a trigger:

create trigger commits_insert_pr_branch after insert on commits

An explain (analyze, buffers of an insert commit query for a large repo shows that a good deal of time is spent inside that trigger function:

Insert on commits  (cost=0.00..0.02 rows=0 width=0) (actual time=36.525..36.526 rows=0 loops=1)
  Buffers: shared hit=278 read=8 dirtied=6
  ->  Result  (cost=0.00..0.02 rows=1 width=654) (actual time=5.910..5.911 rows=1 loops=1)
        Buffers: shared hit=29
Planning:
  Buffers: shared hit=3
Planning Time: 1.979 ms
Trigger for constraint commits_author_fkey: time=13.197 calls=1
Trigger for constraint commits_repoid_fkey: time=1.021 calls=1
Trigger commits_insert_pr_branch: time=32367.409 calls=1
Execution Time: 32423.330 ms

I separately explain analyzed the update repos query for the same repo and it took a very long time.

Links to relevant tickets

N/A

What does this PR do?

Creates a migration to replace the function and omit the update repos query. I don't think it's necessary to do this in the trigger. The value may become slightly out-of-date but the worker will update the value as soon as we start processing the commit.

Marked the migration as risky since we'll plan to run it after hours.

@codecov-staging
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!.

@codecov-qa
Copy link

codecov-qa bot commented Oct 24, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (245a50b) 95.60% compared to head (3fa67d9) 95.60%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #207   +/-   ##
=======================================
  Coverage   95.60%   95.60%           
=======================================
  Files         600      601    +1     
  Lines       15233    15240    +7     
=======================================
+ Hits        14563    14570    +7     
  Misses        670      670           
Flag Coverage Δ
unit 95.60% <100.00%> (+<0.01%) ⬆️
unit-latest-uploader 95.60% <100.00%> (+<0.01%) ⬆️

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.

@codecov-public-qa
Copy link

codecov-public-qa bot commented Oct 24, 2023

Codecov Report

Merging #207 (3fa67d9) into main (245a50b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #207   +/-   ##
=======================================
  Coverage   95.60%   95.60%           
=======================================
  Files         600      601    +1     
  Lines       15233    15240    +7     
=======================================
+ Hits        14563    14570    +7     
  Misses        670      670           
Flag Coverage Δ
unit 95.60% <100.00%> (+<0.01%) ⬆️
unit-latest-uploader 95.60% <100.00%> (+<0.01%) ⬆️

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

Files Coverage Δ
...y_migrations/migrations/0004_auto_20231024_1937.py 100.00% <100.00%> (ø)

Impacted file tree graph

@codecov
Copy link

codecov bot commented Oct 24, 2023

Codecov Report

Merging #207 (168e510) into main (245a50b) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 168e510 differs from pull request most recent head 3fa67d9. Consider uploading reports for the commit 3fa67d9 to get more accurate results

@@           Coverage Diff           @@
##            main    #207     +/-   ##
=======================================
+ Coverage   95.50   95.52   +0.02     
=======================================
  Files        714     715      +1     
  Lines      15639   15635      -4     
=======================================
- Hits       14936   14935      -1     
+ Misses       703     700      -3     
Flag Coverage Δ
unit 95.62% <100.00%> (+0.01%) ⬆️
unit-latest-uploader 95.62% <100.00%> (+0.01%) ⬆️

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

Files Coverage Δ
...y_migrations/migrations/0004_auto_20231024_1937.py 100.00% <100.00%> (ø)

... and 6 files with indirect coverage changes

@scott-codecov scott-codecov merged commit 06a4313 into main Oct 25, 2023
13 of 14 checks passed
@scott-codecov scott-codecov deleted the scott/update-legacy-trigger-function branch October 25, 2023 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants