Repository: qdrant/qdrant
File: lib/shard/src/segment_holder/flush.rs (Rust)

Code before:
        // inconsistent state on disk that is not recoverable after a crash.
        //
        // E.g.: we have a point on an immutable segment. If we use a set-payload operation, we do
        // copy-on-write. The point from immutable segment A is deleted, the updated point is
        // stored on appendable segment B.
        // Because of flush ordering segment B (appendable) is flushed before segment A
        // (not-appendable). If the copy-on-write operation happens in between, the point is
        // deleted from A, but the new point in B is not persisted. We cannot recover this by
        // replaying the WAL in case of a crash because the point in A does not exist anymore,
        // making copy-on-write impossible.
        // Locking all segments prevents copy-on-write operations from occurring in between
        // flushes.
        //
        // WARNING: Ordering is very important here. Specifically:
        // - We MUST lock non-appendable first, then appendable.
        // - We MUST flush according to the copy-on-write dependency graph.
        // Because of this, two rev(erse) calls are used below here.
        //
        // Locking must happen in this order because `apply_points_to_appendable` can take two
        // write locks, also in this order. If we'd use different ordering we will eventually end
        // up with a deadlock.
        let mut segment_reads: Vec<_> = segments
            .iter()
            .map(|segment| Self::aloha_lock_segment_read(segment))
            .collect();

        let max_applied_version = segment_reads.iter().map(|s| s.version()).max().unwrap_or(0);

        if !sync && self.is_background_flushing() {
            // There is already a background flush ongoing, return current max persisted version
            return Ok(self.get_max_persisted_version(segment_reads, lock_order));
        }

        // This lock also prevents multiple parallel sync flushes
        // as it is exclusive
        let mut background_flush_lock = self.lock_flushing()?;

        sort_permutation(&mut segment_reads, &lock_order, |segment_ids| {
            self.sort_segment_ids_by_flush_dependency(segment_ids)
        });

        if sync {
            for read_segment in segment_reads.iter() {
                read_segment.flush(force)?;
            }
            self.flush_dependency
                .lock()
                .retain(|_, _, version| *version > max_applied_version);
        } else {
            let flush_dependency = self.flush_dependency.clone();
            let flushers: Vec<_> = segment_reads

Review comment (bug):
I'm not confident we can just remove this double reverse.

See the comment above. Might be a deadlock footgun.

Code after:
        // inconsistent state on disk that is not recoverable after a crash.
        //
        // E.g.: we have a point on an immutable segment. If we use a set-payload operation, we do
        // copy-on-write. The point from immutable segment A is deleted, the updated point is
        // stored on appendable segment B.
        // Because of flush ordering segment B (appendable) is flushed before segment A
        // (not-appendable). If the copy-on-write operation happens in between, the point is
        // deleted from A, but the new point in B is not persisted. We cannot recover this by
        // replaying the WAL in case of a crash because the point in A does not exist anymore,
        // making copy-on-write impossible.
        // Locking all segments prevents copy-on-write operations from occurring in between
        // flushes.
        //
        // WARNING: Ordering is very important here. Specifically:
        // - We MUST lock non-appendable first, then appendable.
        // - We MUST flush according to the copy-on-write dependency graph.
        // Because of this, two rev(erse) calls are used below here.
        //
        // Locking must happen in this order because `apply_points_to_appendable` can take two
        // write locks, also in this order. If we'd use different ordering we will eventually end
        // up with a deadlock.
        let mut segment_reads: Vec<_> = segments
            .iter()
            .map(|segment| Self::aloha_lock_segment_read(segment))
            .collect();

        let max_applied_version = segment_reads.iter().map(|s| s.version()).max().unwrap_or(0);

        if !sync && self.is_background_flushing() {
            // There is already a background flush ongoing, return current max persisted version
            return Ok(self.get_max_persisted_version(segment_reads, lock_order));
        }

        // This lock also prevents multiple parallel sync flushes
        // as it is exclusive
        let mut background_flush_lock = self.lock_flushing()?;

        sort_permutation(&mut segment_reads, &lock_order, |segment_ids| {
            self.sort_segment_ids_by_flush_dependency(segment_ids)
        });

        // Capture all flushers first to improve data consistency
        let flushers: Vec<_> = segment_reads
            .iter()
            .filter_map(|read_segment| read_segment.flusher(force))
            .collect();

        if sync {
            for flusher in flushers {
                flusher()?;
            }