fix: find gc cutoff points without holding Tenant::gc_cs #7585
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2880 tests run: 2759 passed, 0 failed, 121 skipped (full report)Code coverage* (full report)
* collected from Rust tests only The comment gets automatically updated with the latest test results
90ece17 at 2024-05-03T12:15:53.035Z :recycle: |
koivunej
force-pushed
the
joonas/find_gc_cutoffs_without_gc_cs
branch
from
3c979ad
to
7d161ca
Compare
This was
linked to
issues
May 2, 2024
problame
approved these changes
May 3, 2024
There was a problem hiding this comment.
refresh_gc_info_internal: I don't like that we now have a timelines variable that only contains the timelines for which we produced GcInfo, but later also use self.timelines. I think it's correct, but, it's a super easy footgun down the line, because they can be so easily confused. Maybe newtype magic could help.
But, this is not a hard ask. A comment warning future developers about this would be nice, though.
The only thing I'm insistent on is to factor out the questionmark business, as asked for in #7585 (comment)
Approving on that condition.
koivunej
force-pushed
the
joonas/find_gc_cutoffs_without_gc_cs
branch
from
7d161ca
to
c4183eb
Compare
they are most likely just shutting down errors, and we have no types for such.
there is no reason to keep holding on to it for longer.
koivunej
force-pushed
the
joonas/find_gc_cutoffs_without_gc_cs
branch
from
d923e01
to
873f4ab
Compare
koivunej
added a commit
that referenced
this pull request
May 6, 2024
#7585 introduced test case for deletions while synthetic size is being calculated. The test has a race against deletion, but we only accept one outcome. Fix it to accept 404 as well, as we cannot control from outside which outcome happens. Evidence: https://neon-github-public-dev.s3.amazonaws.com/reports/pr-7456/8970595458/index.html#/testresult/32a5b2f8c4094bdb
conradludgate
pushed a commit
that referenced
this pull request
May 8, 2024
The current implementation of finding timeline gc cutoff Lsn(s) is done while holding `Tenant::gc_cs`. In recent incidents long create branch times were caused by holding the `Tenant::gc_cs` over extremely long `Timeline::find_lsn_by_timestamp`. The fix is to find the GC cutoff values before taking the `Tenant::gc_cs` lock. This change is safe to do because the GC cutoff values and the branch points have no dependencies on each other. In the case of `Timeline::find_gc_cutoff` taking a long time with this change, we should no longer see `Tenant::gc_cs` interfering with branch creation. Additionally, the `Tenant::refresh_gc_info` is now tolerant of timeline deletions (or any other failures to find the pitr_cutoff). This helps with the synthetic size calculation being constantly completed instead of having a break for a timely timeline deletion. Fixes: #7560 Fixes: #7587
conradludgate
pushed a commit
that referenced
this pull request
May 8, 2024
#7585 introduced test case for deletions while synthetic size is being calculated. The test has a race against deletion, but we only accept one outcome. Fix it to accept 404 as well, as we cannot control from outside which outcome happens. Evidence: https://neon-github-public-dev.s3.amazonaws.com/reports/pr-7456/8970595458/index.html#/testresult/32a5b2f8c4094bdb
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The current implementation of finding timeline gc cutoff Lsn(s) is done while holding
Tenant::gc_cs. In recent incidents long create branch times were caused by holding theTenant::gc_csover extremely longTimeline::find_lsn_by_timestamp. The fix is to find the GC cutoff values before taking theTenant::gc_cslock. This change is safe to do because the GC cutoff values and the branch points have no dependencies on each other. In the case ofTimeline::find_gc_cutofftaking a long time with this change, we should no longer seeTenant::gc_csinterfering with branch creation.Additionally, the
Tenant::refresh_gc_infois now tolerant of timeline deletions (or any other failures to find the pitr_cutoff). This helps with the synthetic size calculation being constantly completed instead of having a break for a timely timeline deletion.Fixes: #7560
Fixes: #7587