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

pageserver: coalesce index uploads when possible #10248

Merged
merged 2 commits into from
Jan 14, 2025

Conversation

erikgrinaker
Copy link
Contributor

@erikgrinaker erikgrinaker commented Dec 30, 2024

Problem

With upload queue reordering in #10218, we can easily get into a situation where multiple index uploads are queued back to back, which can't be parallelized. This will happen e.g. when multiple layer flushes enqueue layer/index/layer/index/... and the layers skip the queue and are uploaded in parallel.

These index uploads will incur serial S3 roundtrip latencies, and may block later operations.

Touches #10096.

Summary of changes

When multiple back-to-back index uploads are ready to upload, only upload the most recent index and drop the rest.

Copy link

github-actions bot commented Dec 30, 2024

7304 tests run: 6933 passed, 0 failed, 371 skipped (full report)


Flaky tests (1)

Postgres 17

Code coverage* (full report)

  • functions: 32.9% (8146 of 24779 functions)
  • lines: 48.1% (68268 of 141935 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
17120eb at 2025-01-14T20:29:17.955Z :recycle:

@erikgrinaker erikgrinaker force-pushed the erik/upload-reorder branch 9 times, most recently from 91be562 to dad6139 Compare January 2, 2025 14:46
@erikgrinaker erikgrinaker force-pushed the erik/upload-index-coalesce branch from 06f1d7a to 76698d5 Compare January 2, 2025 15:07
@erikgrinaker erikgrinaker force-pushed the erik/upload-reorder branch 3 times, most recently from e207145 to 419019e Compare January 4, 2025 09:07
@erikgrinaker erikgrinaker force-pushed the erik/upload-index-coalesce branch from 76698d5 to 9d54d23 Compare January 4, 2025 09:20
@erikgrinaker erikgrinaker marked this pull request as ready for review January 4, 2025 09:21
@erikgrinaker erikgrinaker requested a review from a team as a code owner January 4, 2025 09:21
@erikgrinaker erikgrinaker force-pushed the erik/upload-index-coalesce branch from 9d54d23 to 1568f90 Compare January 4, 2025 09:37
Copy link
Contributor

@VladLazar VladLazar left a comment

Choose a reason for hiding this comment

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

Coalesced indices are always in the same generation, so this feels pretty safe.
Coalescing won't kick in until the flush loop stops inserting barriers though, right?

@erikgrinaker
Copy link
Contributor Author

Coalescing won't kick in until the flush loop stops inserting barriers though, right?

That's right.

@erikgrinaker
Copy link
Contributor Author

Coalesced indices are always in the same generation, so this feels pretty safe.

Seems worth making that explicit, I'll add a condition.

@erikgrinaker
Copy link
Contributor Author

Coalesced indices are always in the same generation, so this feels pretty safe.

Seems worth making that explicit, I'll add a condition.

Can't check this, since IndexPart doesn't have the generation, but it must be true since a timeline instance has a static generation. Added a comment.

Base automatically changed from erik/upload-reorder to main January 14, 2025 16:32
@erikgrinaker erikgrinaker force-pushed the erik/upload-index-coalesce branch from 69b35de to a584c4a Compare January 14, 2025 16:47
@erikgrinaker erikgrinaker force-pushed the erik/upload-index-coalesce branch from a584c4a to 17120eb Compare January 14, 2025 18:06
@erikgrinaker erikgrinaker added this pull request to the merge queue Jan 14, 2025
Merged via the queue into main with commit 6debb49 Jan 14, 2025
84 of 88 checks passed
@erikgrinaker erikgrinaker deleted the erik/upload-index-coalesce branch January 14, 2025 21:11
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.

2 participants