From 6b51a16b7bca1605d991c19465a6158a813d5c72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arpad=20M=C3=BCller?= Date: Sat, 6 Jul 2024 01:25:06 +0200 Subject: [PATCH 1/2] Remove ImageCompressionAlgorithm::DisabledNoDecompress --- libs/pageserver_api/src/models.rs | 14 -------------- pageserver/src/config.rs | 2 +- pageserver/src/tenant/blob_io.rs | 11 +++-------- pageserver/src/tenant/block_io.rs | 10 +--------- .../src/tenant/storage_layer/delta_layer.rs | 2 +- .../src/tenant/storage_layer/image_layer.rs | 19 ++++++------------- pageserver/src/tenant/storage_layer/layer.rs | 1 - 7 files changed, 12 insertions(+), 47 deletions(-) diff --git a/libs/pageserver_api/src/models.rs b/libs/pageserver_api/src/models.rs index 49c942938dfd..d360cc6e870f 100644 --- a/libs/pageserver_api/src/models.rs +++ b/libs/pageserver_api/src/models.rs @@ -440,9 +440,6 @@ pub enum CompactionAlgorithm { #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] pub enum ImageCompressionAlgorithm { - /// Disabled for writes, and never decompress during reading. - /// Never set this after you've enabled compression once! - DisabledNoDecompress, // Disabled for writes, support decompressing during read path Disabled, /// Zstandard compression. Level 0 means and None mean the same (default level). Levels can be negative as well. @@ -452,12 +449,6 @@ pub enum ImageCompressionAlgorithm { }, } -impl ImageCompressionAlgorithm { - pub fn allow_decompression(&self) -> bool { - !matches!(self, ImageCompressionAlgorithm::DisabledNoDecompress) - } -} - impl FromStr for ImageCompressionAlgorithm { type Err = anyhow::Error; fn from_str(s: &str) -> Result { @@ -466,7 +457,6 @@ impl FromStr for ImageCompressionAlgorithm { .next() .ok_or_else(|| anyhow::anyhow!("empty string"))?; match first { - "disabled-no-decompress" => Ok(ImageCompressionAlgorithm::DisabledNoDecompress), "disabled" => Ok(ImageCompressionAlgorithm::Disabled), "zstd" => { let level = if let Some(v) = components.next() { @@ -1683,10 +1673,6 @@ mod tests { ImageCompressionAlgorithm::from_str("disabled").unwrap(), Disabled ); - assert_eq!( - ImageCompressionAlgorithm::from_str("disabled-no-decompress").unwrap(), - DisabledNoDecompress - ); assert_eq!( ImageCompressionAlgorithm::from_str("zstd").unwrap(), Zstd { level: None } diff --git a/pageserver/src/config.rs b/pageserver/src/config.rs index b7c9af224404..17bc427b2cf1 100644 --- a/pageserver/src/config.rs +++ b/pageserver/src/config.rs @@ -92,7 +92,7 @@ pub mod defaults { pub const DEFAULT_MAX_VECTORED_READ_BYTES: usize = 128 * 1024; // 128 KiB pub const DEFAULT_IMAGE_COMPRESSION: ImageCompressionAlgorithm = - ImageCompressionAlgorithm::DisabledNoDecompress; + ImageCompressionAlgorithm::Disabled; pub const DEFAULT_VALIDATE_VECTORED_GET: bool = true; diff --git a/pageserver/src/tenant/blob_io.rs b/pageserver/src/tenant/blob_io.rs index 0705182d5db2..e98ed66ef998 100644 --- a/pageserver/src/tenant/blob_io.rs +++ b/pageserver/src/tenant/blob_io.rs @@ -273,12 +273,8 @@ impl BlobWriter { srcbuf: B, ctx: &RequestContext, ) -> (B::Buf, Result) { - self.write_blob_maybe_compressed( - srcbuf, - ctx, - ImageCompressionAlgorithm::DisabledNoDecompress, - ) - .await + self.write_blob_maybe_compressed(srcbuf, ctx, ImageCompressionAlgorithm::Disabled) + .await } /// Write a blob of data. Returns the offset that it was written to, @@ -340,8 +336,7 @@ impl BlobWriter { (BYTE_UNCOMPRESSED, len, slice.into_inner()) } } - ImageCompressionAlgorithm::Disabled - | ImageCompressionAlgorithm::DisabledNoDecompress => { + ImageCompressionAlgorithm::Disabled => { (BYTE_UNCOMPRESSED, len, srcbuf.slice_full().into_inner()) } }; diff --git a/pageserver/src/tenant/block_io.rs b/pageserver/src/tenant/block_io.rs index 3324e840ecf1..601b09515519 100644 --- a/pageserver/src/tenant/block_io.rs +++ b/pageserver/src/tenant/block_io.rs @@ -202,18 +202,10 @@ pub struct FileBlockReader<'a> { impl<'a> FileBlockReader<'a> { pub fn new(file: &'a VirtualFile, file_id: FileId) -> Self { - Self::new_with_compression(file, file_id, false) - } - - pub fn new_with_compression( - file: &'a VirtualFile, - file_id: FileId, - compressed_reads: bool, - ) -> Self { FileBlockReader { file_id, file, - compressed_reads, + compressed_reads: true, } } diff --git a/pageserver/src/tenant/storage_layer/delta_layer.rs b/pageserver/src/tenant/storage_layer/delta_layer.rs index 685f6dce60e7..000076d7c09d 100644 --- a/pageserver/src/tenant/storage_layer/delta_layer.rs +++ b/pageserver/src/tenant/storage_layer/delta_layer.rs @@ -453,7 +453,7 @@ impl DeltaLayerWriterInner { ) -> (Vec, anyhow::Result<()>) { assert!(self.lsn_range.start <= lsn); // We don't want to use compression in delta layer creation - let compression = ImageCompressionAlgorithm::DisabledNoDecompress; + let compression = ImageCompressionAlgorithm::Disabled; let (val, res) = self .blob_writer .write_blob_maybe_compressed(val, ctx, compression) diff --git a/pageserver/src/tenant/storage_layer/image_layer.rs b/pageserver/src/tenant/storage_layer/image_layer.rs index 4a1b3a02377a..33bce4e7ea80 100644 --- a/pageserver/src/tenant/storage_layer/image_layer.rs +++ b/pageserver/src/tenant/storage_layer/image_layer.rs @@ -165,7 +165,6 @@ pub struct ImageLayerInner { file_id: FileId, max_vectored_read_bytes: Option, - compressed_reads: bool, } impl std::fmt::Debug for ImageLayerInner { @@ -180,7 +179,7 @@ impl std::fmt::Debug for ImageLayerInner { impl ImageLayerInner { pub(super) async fn dump(&self, ctx: &RequestContext) -> anyhow::Result<()> { let block_reader = - FileBlockReader::new_with_compression(&self.file, self.file_id, self.compressed_reads); + FileBlockReader::new(&self.file, self.file_id); let tree_reader = DiskBtreeReader::<_, KEY_SIZE>::new( self.index_start_blk, self.index_root_blk, @@ -269,7 +268,7 @@ impl ImageLayer { let path = self.path(); let loaded = - ImageLayerInner::load(&path, self.desc.image_layer_lsn(), None, None, false, ctx) + ImageLayerInner::load(&path, self.desc.image_layer_lsn(), None, None, ctx) .await .and_then(|res| res)?; @@ -380,7 +379,6 @@ impl ImageLayerInner { lsn: Lsn, summary: Option, max_vectored_read_bytes: Option, - support_compressed_reads: bool, ctx: &RequestContext, ) -> Result, anyhow::Error> { let file = match VirtualFile::open(path, ctx).await { @@ -424,7 +422,6 @@ impl ImageLayerInner { file, file_id, max_vectored_read_bytes, - compressed_reads: support_compressed_reads, key_range: actual_summary.key_range, })) } @@ -435,8 +432,7 @@ impl ImageLayerInner { reconstruct_state: &mut ValueReconstructState, ctx: &RequestContext, ) -> anyhow::Result { - let block_reader = - FileBlockReader::new_with_compression(&self.file, self.file_id, self.compressed_reads); + let block_reader = FileBlockReader::new(&self.file, self.file_id); let tree_reader = DiskBtreeReader::new(self.index_start_blk, self.index_root_blk, &block_reader); @@ -496,14 +492,12 @@ impl ImageLayerInner { &self, ctx: &RequestContext, ) -> anyhow::Result> { - let block_reader = - FileBlockReader::new_with_compression(&self.file, self.file_id, self.compressed_reads); + let block_reader = FileBlockReader::new(&self.file, self.file_id); let tree_reader = DiskBtreeReader::new(self.index_start_blk, self.index_root_blk, &block_reader); let mut result = Vec::new(); let mut stream = Box::pin(tree_reader.into_stream(&[0; KEY_SIZE], ctx)); - let block_reader = - FileBlockReader::new_with_compression(&self.file, self.file_id, self.compressed_reads); + let block_reader = FileBlockReader::new(&self.file, self.file_id); let cursor = block_reader.block_cursor(); while let Some(item) = stream.next().await { // TODO: dedup code with get_reconstruct_value @@ -538,8 +532,7 @@ impl ImageLayerInner { .into(), ); - let block_reader = - FileBlockReader::new_with_compression(&self.file, self.file_id, self.compressed_reads); + let block_reader = FileBlockReader::new(&self.file, self.file_id); let tree_reader = DiskBtreeReader::new(self.index_start_blk, self.index_root_blk, block_reader); diff --git a/pageserver/src/tenant/storage_layer/layer.rs b/pageserver/src/tenant/storage_layer/layer.rs index afd11780e77d..02069c29d264 100644 --- a/pageserver/src/tenant/storage_layer/layer.rs +++ b/pageserver/src/tenant/storage_layer/layer.rs @@ -1685,7 +1685,6 @@ impl DownloadedLayer { lsn, summary, Some(owner.conf.max_vectored_read_bytes), - owner.conf.image_compression.allow_decompression(), ctx, ) .await From 46e53a276002d6701a50d7349032254e680d0185 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arpad=20M=C3=BCller?= Date: Sat, 6 Jul 2024 01:30:51 +0200 Subject: [PATCH 2/2] fix test --- pageserver/src/tenant/storage_layer/image_layer.rs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/pageserver/src/tenant/storage_layer/image_layer.rs b/pageserver/src/tenant/storage_layer/image_layer.rs index 33bce4e7ea80..50aacbd9ad46 100644 --- a/pageserver/src/tenant/storage_layer/image_layer.rs +++ b/pageserver/src/tenant/storage_layer/image_layer.rs @@ -178,8 +178,7 @@ impl std::fmt::Debug for ImageLayerInner { impl ImageLayerInner { pub(super) async fn dump(&self, ctx: &RequestContext) -> anyhow::Result<()> { - let block_reader = - FileBlockReader::new(&self.file, self.file_id); + let block_reader = FileBlockReader::new(&self.file, self.file_id); let tree_reader = DiskBtreeReader::<_, KEY_SIZE>::new( self.index_start_blk, self.index_root_blk, @@ -267,10 +266,9 @@ impl ImageLayer { async fn load_inner(&self, ctx: &RequestContext) -> Result { let path = self.path(); - let loaded = - ImageLayerInner::load(&path, self.desc.image_layer_lsn(), None, None, ctx) - .await - .and_then(|res| res)?; + let loaded = ImageLayerInner::load(&path, self.desc.image_layer_lsn(), None, None, ctx) + .await + .and_then(|res| res)?; // not production code let actual_layer_name = LayerName::from_str(path.file_name().unwrap()).unwrap(); @@ -693,8 +691,7 @@ impl ImageLayerInner { #[cfg(test)] pub(crate) fn iter<'a>(&'a self, ctx: &'a RequestContext) -> ImageLayerIterator<'a> { - let block_reader = - FileBlockReader::new_with_compression(&self.file, self.file_id, self.compressed_reads); + let block_reader = FileBlockReader::new(&self.file, self.file_id); let tree_reader = DiskBtreeReader::new(self.index_start_blk, self.index_root_blk, block_reader); ImageLayerIterator {