From db2a5bbe5cdf5e1b47e90cf82dc4a7ebfc2bbe1d Mon Sep 17 00:00:00 2001 From: Andrey Vasnetsov Date: Thu, 13 Aug 2026 00:10:09 +0200 Subject: [PATCH] Move delete-only tombstoning into per-format update-only id trackers (#10212) * Move delete-only tombstoning into per-format update-only id trackers DeleteOnlySegment::tombstone_points wrote the deleted mask file directly, hard-coding that both immutable id-tracker formats store it the same way. Give each immutable format (in-RAM and disk-resident) its own update-only tracker that owns the decision of where its tombstones go, and dispatch through DeleteOnlyIdTrackerEnum, which lives next to ReadOnlyIdTrackerEnum. Both formats share one deleted-mask file today, so the trackers delegate to a shared writer in deleted_storage.rs; a format that diverges later changes only its own update_only module. Co-Authored-By: Claude Fable 5 * Enforce empty-batch guard in the shared tombstone writer Co-Authored-By: Claude Fable 5 --------- Co-authored-by: Claude Fable 5 --- .../src/id_tracker/disk_id_tracker/mod.rs | 1 + .../id_tracker/disk_id_tracker/update_only.rs | 54 +++++++++++ .../delete_only_tracker_enum.rs | 28 ++++++ .../src/id_tracker/id_tracker_base/mod.rs | 1 + .../immutable_id_tracker/deleted_storage.rs | 53 +++++++++++ .../id_tracker/immutable_id_tracker/mod.rs | 3 +- .../immutable_id_tracker/update_only.rs | 54 +++++++++++ .../segment/update_only/delete_only/mod.rs | 89 ++++++------------- .../src/segment/update_only/lookup/mod.rs | 12 ++- lib/segment/src/segment/update_only/mod.rs | 15 ++-- 10 files changed, 235 insertions(+), 75 deletions(-) create mode 100644 lib/segment/src/id_tracker/disk_id_tracker/update_only.rs create mode 100644 lib/segment/src/id_tracker/id_tracker_base/delete_only_tracker_enum.rs create mode 100644 lib/segment/src/id_tracker/immutable_id_tracker/update_only.rs diff --git a/lib/segment/src/id_tracker/disk_id_tracker/mod.rs b/lib/segment/src/id_tracker/disk_id_tracker/mod.rs index 60337de5d5..6de3972eb1 100644 --- a/lib/segment/src/id_tracker/disk_id_tracker/mod.rs +++ b/lib/segment/src/id_tracker/disk_id_tracker/mod.rs @@ -16,6 +16,7 @@ pub mod mappings; pub mod on_disk_format; pub mod read_only; mod reader; +pub mod update_only; #[cfg(test)] mod tests; diff --git a/lib/segment/src/id_tracker/disk_id_tracker/update_only.rs b/lib/segment/src/id_tracker/disk_id_tracker/update_only.rs new file mode 100644 index 0000000000..cbff9320c8 --- /dev/null +++ b/lib/segment/src/id_tracker/disk_id_tracker/update_only.rs @@ -0,0 +1,54 @@ +//! The write half of the disk-resident id tracker for an update-only segment. + +use std::path::{Path, PathBuf}; + +use common::bitvec::BitVec; +use common::types::PointOffsetType; +use common::universal_io::{UniversalRead, UniversalWriteFileOps}; + +use crate::common::operation_error::OperationResult; +use crate::id_tracker::immutable_id_tracker::tombstone_points_in_stored_mask; +use crate::types::PointIdType; + +/// The mapping is frozen, so the one thing to write is the stored deleted +/// mask — which the disk-resident format keeps in the same file as the in-RAM +/// immutable format, hence the shared implementation. Needs only reads plus +/// [`atomic_save`] from the backend, so object stores qualify. +/// +/// [`atomic_save`]: UniversalWriteFileOps::atomic_save +pub struct UpdateOnlyDiskIdTracker + 'static> { + fs: S::Fs, + segment_path: PathBuf, + /// Consumed by the first [`tombstone_points`](Self::tombstone_points) in + /// place of reading the mask file. + deleted: Option, +} + +impl + 'static> UpdateOnlyDiskIdTracker { + /// `deleted` is the mask as the read phase held it in memory, when it + /// did; nothing is read here. + pub fn new(fs: S::Fs, segment_path: &Path, deleted: Option) -> Self { + Self { + fs, + segment_path: segment_path.to_path_buf(), + deleted, + } + } + + /// Retire the given points by marking the slots they occupy in the stored + /// deleted mask — the only thing written, the data on those slots stays. + /// The mask is replaced whole, see [`StoredBitSlice::atomic_update`]. + /// + /// [`StoredBitSlice::atomic_update`]: common::stored_bitslice::StoredBitSlice::atomic_update + pub fn tombstone_points( + &mut self, + points: &[(PointIdType, PointOffsetType)], + ) -> OperationResult<()> { + tombstone_points_in_stored_mask::( + &self.fs, + &self.segment_path, + &mut self.deleted, + points, + ) + } +} diff --git a/lib/segment/src/id_tracker/id_tracker_base/delete_only_tracker_enum.rs b/lib/segment/src/id_tracker/id_tracker_base/delete_only_tracker_enum.rs new file mode 100644 index 0000000000..185fcc3505 --- /dev/null +++ b/lib/segment/src/id_tracker/id_tracker_base/delete_only_tracker_enum.rs @@ -0,0 +1,28 @@ +use common::types::PointOffsetType; +use common::universal_io::{UniversalRead, UniversalWriteFileOps}; + +use crate::common::operation_error::OperationResult; +use crate::id_tracker::disk_id_tracker::update_only::UpdateOnlyDiskIdTracker; +use crate::id_tracker::immutable_id_tracker::update_only::UpdateOnlyImmutableIdTracker; +use crate::types::PointIdType; + +/// The update-only tracker of whichever immutable id-tracker format a segment +/// holds. Each variant decides where its tombstones go. +pub enum DeleteOnlyIdTrackerEnum + 'static> { + Immutable(UpdateOnlyImmutableIdTracker), + DiskResident(UpdateOnlyDiskIdTracker), +} + +impl + 'static> DeleteOnlyIdTrackerEnum { + /// Retire the given points by marking the slots they occupy in the stored + /// deleted mask — the only thing written, the data on those slots stays. + pub fn tombstone_points( + &mut self, + points: &[(PointIdType, PointOffsetType)], + ) -> OperationResult<()> { + match self { + Self::Immutable(id_tracker) => id_tracker.tombstone_points(points), + Self::DiskResident(id_tracker) => id_tracker.tombstone_points(points), + } + } +} diff --git a/lib/segment/src/id_tracker/id_tracker_base/mod.rs b/lib/segment/src/id_tracker/id_tracker_base/mod.rs index 0134155ca0..d5a4f1babe 100644 --- a/lib/segment/src/id_tracker/id_tracker_base/mod.rs +++ b/lib/segment/src/id_tracker/id_tracker_base/mod.rs @@ -2,6 +2,7 @@ mod point_mappings_ref; mod tracker_enum; mod trait_def; +pub mod delete_only_tracker_enum; pub mod read_only_tracker_enum; pub use point_mappings_ref::{PointMappingsGuard, PointMappingsRefEnum}; diff --git a/lib/segment/src/id_tracker/immutable_id_tracker/deleted_storage.rs b/lib/segment/src/id_tracker/immutable_id_tracker/deleted_storage.rs index f97d392f2f..3bcca2e90d 100644 --- a/lib/segment/src/id_tracker/immutable_id_tracker/deleted_storage.rs +++ b/lib/segment/src/id_tracker/immutable_id_tracker/deleted_storage.rs @@ -1,7 +1,60 @@ use std::path::{Path, PathBuf}; +use common::bitvec::BitVec; +use common::mmap::AdviceSetting; +use common::stored_bitslice::StoredBitSlice; +use common::types::PointOffsetType; +use common::universal_io::{OpenOptions, Populate, UniversalRead, UniversalWriteFileOps}; + +use crate::common::operation_error::{OperationError, OperationResult}; +use crate::types::PointIdType; + pub const DELETED_FILE_NAME: &str = "id_tracker.deleted"; pub(crate) fn deleted_path(base: &Path) -> PathBuf { base.join(DELETED_FILE_NAME) } + +/// Retire `points` by setting the slots they occupy in the stored deleted +/// mask of the segment at `segment_path`. The mask is replaced whole, see +/// [`StoredBitSlice::atomic_update`]; `seed` is consumed in place of reading +/// the mask file when the caller held it in memory — and kept for a later +/// call when `points` is empty and nothing is written. +pub(crate) fn tombstone_points_in_stored_mask>( + fs: &S::Fs, + segment_path: &Path, + seed: &mut Option, + points: &[(PointIdType, PointOffsetType)], +) -> OperationResult<()> { + if points.is_empty() { + return Ok(()); + } + StoredBitSlice::::atomic_update( + fs, + deleted_path(segment_path), + OpenOptions { + writeable: false, + need_sequential: false, + populate: Populate::No, + advice: AdviceSetting::Global, + }, + Default::default(), + seed.take(), + |mask| { + for &(point_id, internal_id) in points { + let slot = internal_id as usize; + // A slot beyond the mask names a point this segment cannot hold. + if slot >= mask.len() { + return Err(OperationError::service_error(format!( + "cannot tombstone point {point_id} of segment {}: slot {internal_id} \ + is beyond its deleted mask ({} slots)", + segment_path.display(), + mask.len(), + ))); + } + mask.set(slot, true); + } + Ok(()) + }, + )? +} diff --git a/lib/segment/src/id_tracker/immutable_id_tracker/mod.rs b/lib/segment/src/id_tracker/immutable_id_tracker/mod.rs index 3aafbe4eac..f7d5ea0469 100644 --- a/lib/segment/src/id_tracker/immutable_id_tracker/mod.rs +++ b/lib/segment/src/id_tracker/immutable_id_tracker/mod.rs @@ -6,6 +6,7 @@ mod versions_storage; pub(super) mod tests; pub mod read_only; +pub mod update_only; use std::fmt::Debug; use std::io::{BufReader, BufWriter, Write}; @@ -23,7 +24,7 @@ use common::universal_io::{ use fs_err::File; pub use self::deleted_storage::DELETED_FILE_NAME; -pub(crate) use self::deleted_storage::deleted_path; +pub(crate) use self::deleted_storage::{deleted_path, tombstone_points_in_stored_mask}; pub use self::mappings_storage::{MAPPINGS_FILE_NAME, mappings_path}; use self::mappings_storage::{load_mapping, store_mapping}; pub use self::versions_storage::VERSION_MAPPING_FILE_NAME; diff --git a/lib/segment/src/id_tracker/immutable_id_tracker/update_only.rs b/lib/segment/src/id_tracker/immutable_id_tracker/update_only.rs new file mode 100644 index 0000000000..7682157f73 --- /dev/null +++ b/lib/segment/src/id_tracker/immutable_id_tracker/update_only.rs @@ -0,0 +1,54 @@ +//! The write half of the in-RAM immutable id tracker for an update-only +//! segment. + +use std::path::{Path, PathBuf}; + +use common::bitvec::BitVec; +use common::types::PointOffsetType; +use common::universal_io::{UniversalRead, UniversalWriteFileOps}; + +use super::deleted_storage::tombstone_points_in_stored_mask; +use crate::common::operation_error::OperationResult; +use crate::types::PointIdType; + +/// The mapping is frozen, so the one thing to write is the stored deleted +/// mask ([`DELETED_FILE_NAME`](super::DELETED_FILE_NAME)). Needs only reads +/// plus [`atomic_save`] from the backend, so object stores qualify. +/// +/// [`atomic_save`]: UniversalWriteFileOps::atomic_save +pub struct UpdateOnlyImmutableIdTracker + 'static> { + fs: S::Fs, + segment_path: PathBuf, + /// Consumed by the first [`tombstone_points`](Self::tombstone_points) in + /// place of reading the mask file. + deleted: Option, +} + +impl + 'static> UpdateOnlyImmutableIdTracker { + /// `deleted` is the mask as the read phase held it in memory, when it + /// did; nothing is read here. + pub fn new(fs: S::Fs, segment_path: &Path, deleted: Option) -> Self { + Self { + fs, + segment_path: segment_path.to_path_buf(), + deleted, + } + } + + /// Retire the given points by marking the slots they occupy in the stored + /// deleted mask — the only thing written, the data on those slots stays. + /// The mask is replaced whole, see [`StoredBitSlice::atomic_update`]. + /// + /// [`StoredBitSlice::atomic_update`]: common::stored_bitslice::StoredBitSlice::atomic_update + pub fn tombstone_points( + &mut self, + points: &[(PointIdType, PointOffsetType)], + ) -> OperationResult<()> { + tombstone_points_in_stored_mask::( + &self.fs, + &self.segment_path, + &mut self.deleted, + points, + ) + } +} diff --git a/lib/segment/src/segment/update_only/delete_only/mod.rs b/lib/segment/src/segment/update_only/delete_only/mod.rs index ae23920f51..e0f30ee1df 100644 --- a/lib/segment/src/segment/update_only/delete_only/mod.rs +++ b/lib/segment/src/segment/update_only/delete_only/mod.rs @@ -1,87 +1,56 @@ //! The write phase for an immutable segment: retiring points, and nothing //! else. -use std::path::{Path, PathBuf}; +use std::path::Path; -use common::mmap::AdviceSetting; -use common::stored_bitslice::StoredBitSlice; use common::types::PointOffsetType; -use common::universal_io::{OpenOptions, Populate, UniversalRead, UniversalWriteFileOps}; +use common::universal_io::{UniversalRead, UniversalWriteFileOps}; use super::DeleteOnlyIdTrackerState; -use crate::common::operation_error::{OperationError, OperationResult}; -use crate::id_tracker::immutable_id_tracker::deleted_path; +use crate::common::operation_error::OperationResult; +use crate::id_tracker::delete_only_tracker_enum::DeleteOnlyIdTrackerEnum; +use crate::id_tracker::disk_id_tracker::update_only::UpdateOnlyDiskIdTracker; +use crate::id_tracker::immutable_id_tracker::update_only::UpdateOnlyImmutableIdTracker; use crate::types::PointIdType; /// A segment open for deletes: nothing in it can grow, so the only thing a -/// batch can do here is retire points that are already there. Needs only -/// reads plus [`atomic_save`] from the backend, so object stores qualify. -/// -/// [`atomic_save`]: UniversalWriteFileOps::atomic_save +/// batch can do here is retire points that are already there. Where their +/// tombstones go is the id tracker's decision. pub struct DeleteOnlySegment + 'static> { - fs: S::Fs, - segment_path: PathBuf, - /// Consumed by the first [`tombstone_points`](Self::tombstone_points) in - /// place of reading the mask file. - id_tracker_state: Option, + id_tracker: DeleteOnlyIdTrackerEnum, } impl + 'static> DeleteOnlySegment { - /// Open the segment directory at `segment_path` for deletes; nothing is + /// Open the segment directory at `segment_path` for deletes, resuming the + /// tracker kind its [`DeleteOnlyIdTrackerState`] variant names; nothing is /// read. pub fn open( fs: S::Fs, segment_path: &Path, - id_tracker_state: Option, + id_tracker_state: DeleteOnlyIdTrackerState, ) -> Self { - Self { - fs, - segment_path: segment_path.to_path_buf(), - id_tracker_state, - } + let id_tracker = match id_tracker_state { + DeleteOnlyIdTrackerState::Immutable(deleted) => DeleteOnlyIdTrackerEnum::Immutable( + UpdateOnlyImmutableIdTracker::new(fs, segment_path, deleted), + ), + DeleteOnlyIdTrackerState::DiskResident(deleted) => { + DeleteOnlyIdTrackerEnum::DiskResident(UpdateOnlyDiskIdTracker::new( + fs, + segment_path, + deleted, + )) + } + }; + Self { id_tracker } } - /// Retire the given points by marking the slots they occupy in the - /// deleted-points bitmask (`id_tracker.deleted`) — the only thing - /// written, the data on those slots stays. The mask is replaced whole, - /// see [`StoredBitSlice::atomic_update`]. + /// Retire the given points by marking the slots they occupy in the id + /// tracker's stored deleted mask — the only thing written, the data on + /// those slots stays. pub fn tombstone_points( &mut self, points: &[(PointIdType, PointOffsetType)], ) -> OperationResult<()> { - if points.is_empty() { - return Ok(()); - } - - let seed = self.id_tracker_state.take().map(|state| state.deleted); - - StoredBitSlice::::atomic_update( - &self.fs, - deleted_path(&self.segment_path), - OpenOptions { - writeable: false, - need_sequential: false, - populate: Populate::No, - advice: AdviceSetting::Global, - }, - Default::default(), - seed, - |mask| { - for &(point_id, internal_id) in points { - let slot = internal_id as usize; - // A slot beyond the mask names a point this segment cannot hold. - if slot >= mask.len() { - return Err(OperationError::service_error(format!( - "cannot tombstone point {point_id} of segment {}: slot {internal_id} \ - is beyond its deleted mask ({} slots)", - self.segment_path.display(), - mask.len(), - ))); - } - mask.set(slot, true); - } - Ok(()) - }, - )? + self.id_tracker.tombstone_points(points) } } diff --git a/lib/segment/src/segment/update_only/lookup/mod.rs b/lib/segment/src/segment/update_only/lookup/mod.rs index b63e1a7822..c3c6e67c7f 100644 --- a/lib/segment/src/segment/update_only/lookup/mod.rs +++ b/lib/segment/src/segment/update_only/lookup/mod.rs @@ -56,15 +56,13 @@ impl LookupSegment { }) } ReadOnlyIdTrackerEnum::Immutable(id_tracker) => { - WriterIdTrackerState::DeleteOnly(Some(DeleteOnlyIdTrackerState { - deleted: id_tracker.deleted_point_bitslice().to_bitvec(), - })) + WriterIdTrackerState::DeleteOnly(DeleteOnlyIdTrackerState::Immutable(Some( + id_tracker.deleted_point_bitslice().to_bitvec(), + ))) } ReadOnlyIdTrackerEnum::DiskResident(id_tracker) => { - WriterIdTrackerState::DeleteOnly(id_tracker.deleted_full_if_materialized().map( - |deleted| DeleteOnlyIdTrackerState { - deleted: deleted.clone(), - }, + WriterIdTrackerState::DeleteOnly(DeleteOnlyIdTrackerState::DiskResident( + id_tracker.deleted_full_if_materialized().cloned(), )) } } diff --git a/lib/segment/src/segment/update_only/mod.rs b/lib/segment/src/segment/update_only/mod.rs index 227d19a048..bd51a38ed5 100644 --- a/lib/segment/src/segment/update_only/mod.rs +++ b/lib/segment/src/segment/update_only/mod.rs @@ -39,9 +39,7 @@ use crate::types::PointIdType; /// decides the writer's kind. pub enum WriterIdTrackerState { Appendable(AppendableIdTrackerState), - /// `None` when the read phase did not hold the deleted mask in memory; - /// the writer then reads it itself. - DeleteOnly(Option), + DeleteOnly(DeleteOnlyIdTrackerState), } /// The tail of an appendable segment's mappings log, as the read phase saw it. @@ -59,8 +57,11 @@ pub struct AppendableIdTrackerState { pub mappings_end: u64, } -/// An immutable segment's deleted-points mask as the read phase held it in -/// memory, sparing the writer the read of the mask file. -pub struct DeleteOnlyIdTrackerState { - pub deleted: BitVec, +/// Which immutable id tracker the read phase found — deciding which +/// update-only tracker the writer resumes with — carrying the deleted-points +/// mask when the read phase held it in memory, sparing the writer the read of +/// the mask file. `None` means the writer's tracker reads it itself. +pub enum DeleteOnlyIdTrackerState { + Immutable(Option), + DiskResident(Option), }