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

Implement decompression for vectored reads #8302

Merged
merged 7 commits into from
Jul 12, 2024
Merged

Implement decompression for vectored reads #8302

merged 7 commits into from
Jul 12, 2024

Conversation

arpad-m
Copy link
Member

@arpad-m arpad-m commented Jul 6, 2024 •
edited
Loading

Implement decompression of images for vectored reads.

This doesn't implement support for still treating blobs as uncompressed with the bits we reserved for compression, as we are removing that functionality in #8300 anyways.

Part of #5431

@arpad-m arpad-m mentioned this pull request Jul 6, 2024
Closed
30 tasks
@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 6, 2024 •
edited
Loading

3048 tests run: 2933 passed, 0 failed, 115 skipped (full report)

Flaky tests (3)

Postgres 15

  • test_isolation[None]: debug
  • test_tenant_creation_fails: debug

Postgres 14

Code coverage* (full report)

  • functions: 32.8% (6985 of 21326 functions)
  • lines: 50.2% (54998 of 109620 lines)

* collected from Rust tests only

The comment gets automatically updated with the latest test results
c736123 at 2024-07-12T01:16:00.624Z :recycle:

@arpad-m arpad-m marked this pull request as ready for review July 8, 2024 19:13
@arpad-m arpad-m requested a review from a team as a code owner July 8, 2024 19:13
@arpad-m arpad-m requested a review from VladLazar July 8, 2024 19:13
@jcsp
Copy link
Collaborator

jcsp commented Jul 9, 2024

Flaky test looks like something significant:

fixtures.pageserver.http.PageserverApiException: create image layers

@jcsp jcsp mentioned this pull request Jul 9, 2024
Closed
@arpad-m
Copy link
Member Author

arpad-m commented Jul 9, 2024

Interesting, PS logs say (pageserver_3):

2024-07-08T19:47:17.192071Z ERROR request{method=PUT path=/v1/tenant/f897d0661d7a611c2f8a6e6d02db31d5-0204/timeline/0b988c50497f57d037bb39151ab4b352/checkpoint request_id=d626a796-7c25-4c2d-a8fd-404635bcf075}: Error processing HTTP request: InternalServerError(create image layers

Caused by:
    0: rename temporary file as correct path for 000000067F000035BA00000D7F0100000000-000000067F0000400000000A3A0300000000__00000000159A2270-00000001
    1: EEXIST: File exists

It uploaded that same file a little bit earlier as part of background compaction:

2024-07-08T19:46:54.286512Z  INFO compaction_loop{tenant_id=f897d0661d7a611c2f8a6e6d02db31d5 shard_id=0204}:compact_timeline{timeline_id=0b988c50497f57d037bb39151ab4b352}: scheduled layer file upload 000000067F000035BA00000D7F0100000000-000000067F0000400000000A3A0300000000__00000000159A2270-00000001 gen=00000001 shard=0204

I also see it upload the index_part.json after that message and before the erroring HTTP request:

2024-07-08T19:46:54.546389Z  INFO remote_upload{tenant_id=f897d0661d7a611c2f8a6e6d02db31d5 shard_id=0204 timeline_id=0b988c50497f57d037bb39151ab4b352 upload_task_id=90}: uploaded an index_part.json with future layers -- this is ok! if shutdown now, expect future layer cleanup disk_consistent_lsn=0/158E25D0

It's an interesting race. Maybe compression makes the situation worse due to taking more time, but in general it seems like an unrelated issue to me: there is earlier failures with the same error message (test_pg_regress[4] fails with a number of error messages).

@arpad-m arpad-m requested a review from VladLazar July 9, 2024 14:35
VladLazar
VladLazar approved these changes Jul 11, 2024
View reviewed changes
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.

Looks good. I would personally try a CI run of this PR rebased on #8288, but I leave it up to you.

@arpad-m
Copy link
Member Author

arpad-m commented Jul 11, 2024

I would personally try a CI run of this PR rebased on #8288

before enabling it in staging or prod, I certainly will do that. I'd like to handle any upcoming issues in followups.

@arpad-m arpad-m merged commit 2e37aa3 into main Jul 12, 2024
65 checks passed
@arpad-m arpad-m deleted the arpad/compression_8 branch July 12, 2024 02:32
skyzh pushed a commit that referenced this pull request Jul 15, 2024
@arpad-m @skyzh
Implement decompression for vectored reads (#8302)
0e600eb 
Implement decompression of images for vectored reads.

This doesn't implement support for still treating blobs as uncompressed
with the bits we reserved for compression, as we have removed that
functionality in #8300 anyways.

Part of #5431
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VladLazar VladLazar VladLazar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@arpad-m @jcsp @VladLazar