diff --git a/pkg/storage/replica_command.go b/pkg/storage/replica_command.go index fae38378cc50..d824af3d4d2a 100644 --- a/pkg/storage/replica_command.go +++ b/pkg/storage/replica_command.go @@ -45,6 +45,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/storage/engine" "github.com/cockroachdb/cockroach/pkg/storage/engine/enginepb" "github.com/cockroachdb/cockroach/pkg/storage/storagebase" + "github.com/cockroachdb/cockroach/pkg/util" "github.com/cockroachdb/cockroach/pkg/util/hlc" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/cockroach/pkg/util/protoutil" @@ -1884,16 +1885,38 @@ func evalTruncateLog( if err != nil { return EvalResult{}, err } - start := keys.RaftLogKey(cArgs.EvalCtx.RangeID(), 0) - end := keys.RaftLogKey(cArgs.EvalCtx.RangeID(), args.Index) - var diff enginepb.MVCCStats - // Passing zero timestamp to MVCCDeleteRange is equivalent to a ranged clear - // but it also computes stats. Note that any sideloaded payloads that may be - // removed by this truncation don't matter; they're not tracked in the raft - // log delta. - if _, _, _, err := engine.MVCCDeleteRange(ctx, batch, &diff, start, end, math.MaxInt64, /* max */ - hlc.Timestamp{}, nil /* txn */, false /* returnKeys */); err != nil { - return EvalResult{}, err + + // We start at index zero because it's always possible that a previous + // truncation did not clean up entries made obsolete by the previous + // truncation. + start := engine.MakeMVCCMetadataKey(keys.RaftLogKey(cArgs.EvalCtx.RangeID(), 0)) + end := engine.MakeMVCCMetadataKey(keys.RaftLogKey(cArgs.EvalCtx.RangeID(), args.Index)) + + var ms enginepb.MVCCStats + if util.IsMigrated() { + // Compute the stats delta that were to occur should the log entries be + // purged. We do this as a side effect of seeing a new TruncatedState, + // downstream of Raft. A follower may not run the side effect in the event + // of an ill-timed crash, but that's OK since the next truncation will get + // everything. + // + // Note that any sideloaded payloads that may be removed by this truncation + // don't matter; they're not tracked in the raft log delta. + iter := batch.NewIterator(false /* !prefix */) + defer iter.Close() + // We can pass zero as nowNanos because we're only interested in SysBytes. + var err error + ms, err = iter.ComputeStats(start, end, 0 /* nowNanos */) + if err != nil { + return EvalResult{}, errors.Wrap(err, "while computing stats of Raft log freed by truncation") + } + ms.SysBytes = -ms.SysBytes // simulate the deletion + + } else { + if _, _, _, err := engine.MVCCDeleteRange(ctx, batch, &ms, start.Key, end.Key, math.MaxInt64, /* max */ + hlc.Timestamp{}, nil /* txn */, false /* returnKeys */); err != nil { + return EvalResult{}, err + } } tState := &roachpb.RaftTruncatedState{ @@ -1903,7 +1926,7 @@ func evalTruncateLog( var pd EvalResult pd.Replicated.State.TruncatedState = tState - pd.Replicated.RaftLogDelta = &diff.SysBytes + pd.Replicated.RaftLogDelta = &ms.SysBytes return pd, makeReplicaStateLoader(cArgs.EvalCtx.RangeID()).setTruncatedState(ctx, batch, cArgs.Stats, tState) } diff --git a/pkg/storage/replica_proposal.go b/pkg/storage/replica_proposal.go index e90a3e334a9d..6594babfe4e8 100644 --- a/pkg/storage/replica_proposal.go +++ b/pkg/storage/replica_proposal.go @@ -737,17 +737,54 @@ func (r *Replica) handleReplicatedEvalResult( if newTruncState := rResult.State.TruncatedState; newTruncState != nil { rResult.State.TruncatedState = nil // for assertion + r.mu.Lock() r.mu.state.TruncatedState = newTruncState r.mu.Unlock() + + // TODO(tschottdorf): everything below doesn't need to be on this + // goroutine. Worth moving out -- truncations are frequent and missing + // one of the side effects below doesn't matter. Need to be careful + // about the interaction with `evalTruncateLog` though, which computes + // some stats based on the log entries it sees. Also, sideloaded storage + // needs to hold the raft mu. Perhaps it should just get its own mutex + // (which is usually held together with raftMu, except when accessing + // the storage for a truncation). Or, even better, make use of the fact + // that all we need to synchronize is disk i/o, and there is no overlap + // between files *removed* during truncation and those active in Raft. + + if util.IsMigrated() { + // Truncate the Raft log. + start := engine.MakeMVCCMetadataKey(keys.RaftLogKey(r.RangeID, 0)) + end := engine.MakeMVCCMetadataKey( + keys.RaftLogKey(r.RangeID, newTruncState.Index).PrefixEnd(), + ) + iter := r.store.engine.NewIterator(false /* !prefix */) + // Clear the log entries. Intentionally don't use range deletion + // tombstones (ClearRange()) due to performance concerns connected + // to having many range deletion tombstones. There is a chance that + // ClearRange will perform well here because the tombstones could be + // "collapsed", but it is hardly worth the risk at this point. + err := r.store.engine.ClearIterRange(iter, start, end) + iter.Close() + + if err != nil { + log.Errorf(ctx, "unable to clear truncated Raft entries for %+v: %s", newTruncState, err) + } + } + // Clear any entries in the Raft log entry cache for this range up // to and including the most recently truncated index. r.store.raftEntryCache.clearTo(r.RangeID, newTruncState.Index+1) - log.Eventf(ctx, "truncating sideloaded storage up to (and including) index %d", newTruncState.Index) - if err := r.raftMu.sideloaded.TruncateTo(ctx, newTruncState.Index+1); err != nil { - // We don't *have* to remove these entries for correctness. Log a - // loud error, but keep humming along. - log.Errorf(ctx, "while removing sideloaded files during log truncation: %s", err) + + // Truncate the sideloaded storage. + { + log.Eventf(ctx, "truncating sideloaded storage up to (and including) index %d", newTruncState.Index) + if err := r.raftMu.sideloaded.TruncateTo(ctx, newTruncState.Index+1); err != nil { + // We don't *have* to remove these entries for correctness. Log a + // loud error, but keep humming along. + log.Errorf(ctx, "while removing sideloaded files during log truncation: %s", err) + } } } diff --git a/pkg/util/migration_stub.go b/pkg/util/migration_stub.go index 4e350b0addcd..96527f45b3ba 100644 --- a/pkg/util/migration_stub.go +++ b/pkg/util/migration_stub.go @@ -16,8 +16,6 @@ package util import "github.com/cockroachdb/cockroach/pkg/util/envutil" -var _ = IsMigrated() - // IsMigrated gives a crude way of landing changes that need a migration until // #16977 is implemented. When IsMigrated() returns false (the default), // mixed-cluster compatibility with 1.0 is required.