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

tiered compaction: can't get last key #7287

Closed
Tracked by #7554 ...
arpad-m opened this issue Apr 2, 2024 · 1 comment · Fixed by #7551
Closed
Tracked by #7554 ...

tiered compaction: can't get last key #7287

arpad-m opened this issue Apr 2, 2024 · 1 comment · Fixed by #7551
Assignees
Labels
c/storage/pageserver Component: storage: pageserver

Comments

@arpad-m
Copy link
Member

arpad-m commented Apr 2, 2024

Found in #7283, the test_read_at_max_lsn test added by #7007 is failing with tiered compaction:

diff --git a/pageserver/src/tenant.rs b/pageserver/src/tenant.rs
index 0806ef0cf..70b6eaa44 100644
--- a/pageserver/src/tenant.rs
+++ b/pageserver/src/tenant.rs
@@ -3853,6 +3853,7 @@ mod tests {
     use bytes::BytesMut;
     use hex_literal::hex;
     use pageserver_api::keyspace::KeySpace;
+    use pageserver_api::models::CompactionAlgorithm;
     use rand::{thread_rng, Rng};
 
     static TEST_KEY: Lazy<Key> =
@@ -5151,7 +5152,9 @@ mod tests {
 
     #[tokio::test]
     async fn test_read_at_max_lsn() -> anyhow::Result<()> {
-        let harness = TenantHarness::create("test_read_at_max_lsn")?;
+        let mut harness = TenantHarness::create("test_read_at_max_lsn")?;
+        harness.tenant_conf.compaction_algorithm = CompactionAlgorithm::Tiered;
+
         let (tenant, ctx) = harness.load().await;
         let tline = tenant
             .create_test_timeline(TIMELINE_ID, Lsn(0x08), DEFAULT_PG_VERSION, &ctx)
@@ -5163,7 +5166,8 @@ mod tests {
         let test_key = Key::from_hex("010000000033333333444444445500000000").unwrap();
         let read_lsn = Lsn(u64::MAX - 1);
 
-        assert!(tline.get(test_key, read_lsn, &ctx).await.is_ok());
+        let result = tline.get(test_key, read_lsn, &ctx).await;
+        assert!(result.is_ok(), "result is not Ok: {}", result.unwrap_err());
 
         Ok(())
     }

originally part of #6768
moved to #7554

@arpad-m arpad-m self-assigned this Apr 2, 2024
@arpad-m arpad-m added the c/storage/pageserver Component: storage: pageserver label Apr 2, 2024
@arpad-m
Copy link
Member Author

arpad-m commented Apr 24, 2024

Preliminary results from investigation yesterday and today:

  • the test first writes a bunch of keys, including 010000000033333333444444445500000000 (test_key), runs compaction, and then queries test_key, expecting to find it.
  • legacy compaction finds the key, tiered compaction doesn't.
  • for tiered compaction, it tries to look into an image layer that should contain the key but it doesn't
  • I instrumented collect_keyspace to print the lsn and whether the returned range includes the key or not. It turns out that the returned range doesn't include the key! It is used both by new and old compaction to determine which keys to put into the image layer.
  • However there is a difference between legacy and tiered compaction: collect_keyspace never gets called for any non-start lsns for legacy compaction. in other words, legacy compaction doesn't put test_key into an image but leaves it in a delta.
  • tiered compaction creates an image however, which does call collect_keyspace for an lsn that's supposed to include the key. but the key never ends up in the image layer.

After a chat with @VladLazar things are more clearer now: the test writes the keys via TimelineWriter that bypasses DBDIR_KEY. So that's why they don't show up in collect_keyspace.

It seems to me that this is a faulty test issue rather than a problem with tiered compaction.

I'll try to make the legacy compaction issue an image layer to maybe reproduce it there as well.

arpad-m added a commit that referenced this issue Apr 30, 2024
… compaction (#7551)

Makes two of the tests work with the tiered compaction that I had to
ignore in #7283.

The issue was that tiered compaction actually created image layers, but
the keys didn't appear in them as `collect_keyspace` didn't include
them. Not a compaction problem, but due to how the test is structured.

Fixes #7287
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c/storage/pageserver Component: storage: pageserver
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant