Skip to content

Commit

Permalink
Remove ImageCompressionAlgorithm::DisabledNoDecompress (#8300)
Browse files Browse the repository at this point in the history 
Removes the `ImageCompressionAlgorithm::DisabledNoDecompress` variant.
We now assume any blob with the specific bits set is actually a
compressed blob.

The `ImageCompressionAlgorithm::Disabled` variant still remains and is
the new default.

Reverts large parts of #8238 , as originally intended in that PR.

Part of #5431
  • Loading branch information
@arpad-m
arpad-m committed Jul 10, 2024
1 parent 98387d6 commit e78341e
Show file tree
Hide file tree
Showing 7 changed files with 15 additions and 53 deletions.
14 changes: 0 additions & 14 deletions libs/pageserver_api/src/models.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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<Self, Self::Err> {
Expand All @@ -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() {
Expand Down Expand Up @@ -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 }
Expand Down
2 changes: 1 addition & 1 deletion pageserver/src/config.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
11 changes: 3 additions & 8 deletions pageserver/src/tenant/blob_io.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -273,12 +273,8 @@ impl<const BUFFERED: bool> BlobWriter<BUFFERED> {
srcbuf: B,
ctx: &RequestContext,
) -> (B::Buf, Result<u64, Error>) {
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,
Expand Down Expand Up @@ -340,8 +336,7 @@ impl<const BUFFERED: bool> BlobWriter<BUFFERED> {
(BYTE_UNCOMPRESSED, len, slice.into_inner())
}
}
ImageCompressionAlgorithm::Disabled
| ImageCompressionAlgorithm::DisabledNoDecompress => {
ImageCompressionAlgorithm::Disabled => {
(BYTE_UNCOMPRESSED, len, srcbuf.slice_full().into_inner())
}
};
Expand Down
10 changes: 1 addition & 9 deletions pageserver/src/tenant/block_io.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
}

Expand Down
2 changes: 1 addition & 1 deletion pageserver/src/tenant/storage_layer/delta_layer.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -453,7 +453,7 @@ impl DeltaLayerWriterInner {
) -> (Vec<u8>, 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)
Expand Down
28 changes: 9 additions & 19 deletions pageserver/src/tenant/storage_layer/image_layer.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,6 @@ pub struct ImageLayerInner {
file_id: FileId,

max_vectored_read_bytes: Option<MaxVectoredReadBytes>,
compressed_reads: bool,
}

impl std::fmt::Debug for ImageLayerInner {
Expand All @@ -179,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_with_compression(&self.file, self.file_id, self.compressed_reads);
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,
Expand Down Expand Up @@ -268,10 +266,9 @@ impl ImageLayer {
async fn load_inner(&self, ctx: &RequestContext) -> Result<ImageLayerInner> {
let path = self.path();

let loaded =
ImageLayerInner::load(&path, self.desc.image_layer_lsn(), None, None, false, 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();
Expand Down Expand Up @@ -380,7 +377,6 @@ impl ImageLayerInner {
lsn: Lsn,
summary: Option<Summary>,
max_vectored_read_bytes: Option<MaxVectoredReadBytes>,
support_compressed_reads: bool,
ctx: &RequestContext,
) -> Result<Result<Self, anyhow::Error>, anyhow::Error> {
let file = match VirtualFile::open(path, ctx).await {
Expand Down Expand Up @@ -424,7 +420,6 @@ impl ImageLayerInner {
file,
file_id,
max_vectored_read_bytes,
compressed_reads: support_compressed_reads,
key_range: actual_summary.key_range,
}))
}
Expand All @@ -435,8 +430,7 @@ impl ImageLayerInner {
reconstruct_state: &mut ValueReconstructState,
ctx: &RequestContext,
) -> anyhow::Result<ValueReconstructResult> {
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);

Expand Down Expand Up @@ -496,14 +490,12 @@ impl ImageLayerInner {
&self,
ctx: &RequestContext,
) -> anyhow::Result<Vec<(Key, Lsn, Value)>> {
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
Expand Down Expand Up @@ -538,8 +530,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);

Expand Down Expand Up @@ -700,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 {
Expand Down
1 change: 0 additions & 1 deletion pageserver/src/tenant/storage_layer/layer.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1685,7 +1685,6 @@ impl DownloadedLayer {
lsn,
summary,
Some(owner.conf.max_vectored_read_bytes),
owner.conf.image_compression.allow_decompression(),
ctx,
)
.await
Expand Down

1 comment on commit e78341e

@github-actions
Copy link

Choose a reason for hiding this comment

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

3129 tests run: 3001 passed, 1 failed, 127 skipped (full report)

Failures on Postgres 14

  • test_slow_secondary_downloads[False]: release
# Run all failed tests locally:
scripts/pytest -vv -n $(nproc) -k "test_slow_secondary_downloads[release-pg14-False]"

Test coverage report is not available

The comment gets automatically updated with the latest test results
e78341e at 2024-07-10T17:34:42.309Z :recycle:

Please sign in to comment.