kvstorage: add WAG truncation infrastructure by iskettaneh · Pull Request #167533 · cockroachdb/cockroach

iskettaneh · 2026-04-04T14:11:45Z

This PR adds the infrastructure that will be used to truncating WAG
nodes. It will be used in two places: 1) Truncating all WAG nodes on
startup, and 2) Periodically truncating durably applied WAG nodes.

Release note: None

References: #167607

trunk-io · 2026-04-04T14:11:49Z

😎 Merged successfully - details.

cockroach-teamcity · 2026-04-04T14:11:59Z

This change is

pav-kv

A few quick comments. Will take a closer look tomorrow.

pav-kv · 2026-04-07T21:28:43Z

+		return truncationResult{}, err
+	}
+	if deleted == 0 {
+		return truncationResult{outcome: TruncatedNone}, nil


Is there a difference between TruncatedNone with seen == deleted == 0 and seen == 1 && deleted == 0?

Could you outline for my information how this enum result is going to be used? Alternatively, don't introduce this for now, add later in the PR that would be using it. I'm assuming this would be mainly for the online truncation?

This was assuming that we will be able to remove all WAG nodes in one shot. My idea was that we could use to perform some assertions. For example, at engine startup, we expect ALL wag nodes to be deleted. But now that we are going to limit the number of WAG nodes that can be deleted in one go, I don't think this field helps much.

I see. For this assertion, seems like seen == deleted == 0 should be counted as "all" rather than "none", correct? Maybe move the if deleted == seen check up? Meaning an empty WAG is fully applied by definition.

Possibly, the None case is not interesting. We just want to know if the WAG is fully applied or not?

the startup code cares about WAG being fully applied (not necessarily truncated, btw; maybe in the future we may want to keep it around for a bit, for debuggability)

the runtime truncation might use this as an optimization to know if it should ignore flushes. Though it's quite possible that it will be implemented in some other way in the end, due to concurrency/gaps/etc. E.g. WAG can be empty but some writes already queued up via the Seq. More likely we would be watching the last deleted WAG index vs Seq index for that sort of signal.

I removed that logic and I am just returning a boolean to say whether a deletion happened or not. The startup truncation assertion can be done without this piece.
And for the periodic truncations, I am thinking of this signal:

Have a variable that contains the last WAG index that was successfully deleted.

When we recieve a signal from the state engine that there was a flush, check if the last given seq number is higher than the last deleted WAG node. If not (they are equal), there is no way that there is any WAG node to even check.

When we run a periodic truncation, we can start from the index right after the last successfully truncated WAG node index. This way we don't need to go through previously deleted WAG nodes.

iskettaneh

@iskettaneh made 1 comment.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on pav-kv).

iskettaneh · 2026-04-07T21:45:50Z

+		return truncationResult{}, err
+	}
+	if deleted == 0 {
+		return truncationResult{outcome: TruncatedNone}, nil


This was assuming that we will be able to remove all WAG nodes in one shot. My idea was that we could use to perform some assertions. For example, at engine startup, we expect ALL wag nodes to be deleted. But now that we are going to limit the number of WAG nodes that can be deleted in one go, I don't think this field helps much.

iskettaneh

@iskettaneh made 2 comments.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on pav-kv).

pav-kv · 2026-04-07T23:35:42Z

+		return truncationResult{}, err
+	}
+	if deleted == 0 {
+		return truncationResult{outcome: TruncatedNone}, nil


I see. For this assertion, seems like seen == deleted == 0 should be counted as "all" rather than "none", correct? Maybe move the if deleted == seen check up? Meaning an empty WAG is fully applied by definition.

Possibly, the None case is not interesting. We just want to know if the WAG is fully applied or not?

the startup code cares about WAG being fully applied (not necessarily truncated, btw; maybe in the future we may want to keep it around for a bit, for debuggability)

the runtime truncation might use this as an optimization to know if it should ignore flushes. Though it's quite possible that it will be implemented in some other way in the end, due to concurrency/gaps/etc. E.g. WAG can be empty but some writes already queued up via the Seq. More likely we would be watching the last deleted WAG index vs Seq index for that sort of signal.

iskettaneh

@iskettaneh made 16 comments and resolved 2 discussions.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on pav-kv).

iskettaneh · 2026-04-08T17:53:21Z

+		return truncationResult{}, err
+	}
+	if deleted == 0 {
+		return truncationResult{outcome: TruncatedNone}, nil


I removed that logic and I am just returning a boolean to say whether a deletion happened or not. The startup truncation assertion can be done without this piece.
And for the periodic truncations, I am thinking of this signal:

Have a variable that contains the last WAG index that was successfully deleted.

When we recieve a signal from the state engine that there was a flush, check if the last given seq number is higher than the last deleted WAG node. If not (they are equal), there is no way that there is any WAG node to even check.

When we run a periodic truncation, we can start from the index right after the last successfully truncated WAG node index. This way we don't need to go through previously deleted WAG nodes.

pav-kv

Looks great, thanks for the cleanups. Probably good to merge after rebasing on Mira's PR, I'll review it soon.

pav-kv · 2026-04-08T18:42:27Z

+	sl := MakeStateLoader(rangeID)
+	prefix := sl.RaftLogPrefix()


nit: sl is used only once. Simply use keys.RaftLogPrefix(rangeID) instead?

iskettaneh

@iskettaneh made 4 comments and resolved 5 discussions.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on pav-kv).

This PR adds the infrastructure that will be used to truncating WAG nodes. It will be used in two places: 1) Truncating all WAG nodes on startup, and 2) Periodically truncating durably applied WAG nodes. A lot of functions are similar to WAG replay, and there are TODOs to reconcile the functions between WAG replay and truncation. There is also some work left for clearing the Raft state, and deleting sideloaded files. Release note: None Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>

When deleting a WAG node that has the events destroy or subsume, we also need to delete the raft log (up until and including the addr.index), and any sideloaded files that are part of that raft log prefix. We do this because when a replica delete or subsume is applied, we cannot just yet delete the replica state (log and sideloaded files) because we might crash before the state engine has flushed, and we might need the raft log to replay the event. Release note: None Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>

iskettaneh · 2026-04-09T13:28:22Z

TFTR!

/trunk merge

iskettaneh force-pushed the rse_truncate_2 branch from 66d1a39 to 7755a41 Compare April 5, 2026 14:51

iskettaneh changed the title ~~kvstorage/wag: prepare for WAG truncation~~ kvstorage: add WAG truncation infrastructure Apr 5, 2026

iskettaneh force-pushed the rse_truncate_2 branch from 7755a41 to 0b8c355 Compare April 7, 2026 21:01

iskettaneh marked this pull request as ready for review April 7, 2026 21:03

iskettaneh requested a review from a team as a code owner April 7, 2026 21:03

pav-kv reviewed Apr 7, 2026

View reviewed changes

Comment thread pkg/kv/kvserver/kvstorage/wag_truncator.go Outdated

iskettaneh force-pushed the rse_truncate_2 branch from 0b8c355 to 7150474 Compare April 7, 2026 21:18

pav-kv reviewed Apr 7, 2026

View reviewed changes

iskettaneh commented Apr 7, 2026

View reviewed changes

iskettaneh force-pushed the rse_truncate_2 branch from 7150474 to f006510 Compare April 8, 2026 00:58

iskettaneh commented Apr 8, 2026

View reviewed changes

Comment thread pkg/kv/kvserver/kvstorage/wag_truncator.go Outdated

Comment thread pkg/kv/kvserver/kvstorage/wag_truncator_test.go

pav-kv reviewed Apr 8, 2026

View reviewed changes

iskettaneh force-pushed the rse_truncate_2 branch from f006510 to d3de7ff Compare April 8, 2026 17:53

iskettaneh commented Apr 8, 2026

View reviewed changes

iskettaneh requested a review from pav-kv April 8, 2026 17:53

pav-kv reviewed Apr 8, 2026

View reviewed changes

iskettaneh force-pushed the rse_truncate_2 branch from d3de7ff to 8108b23 Compare April 8, 2026 20:06

iskettaneh commented Apr 8, 2026

View reviewed changes

Comment thread pkg/kv/kvserver/kvstorage/wag_truncator.go

Comment thread pkg/kv/kvserver/kvstorage/wag_truncator.go

Comment thread pkg/kv/kvserver/kvstorage/wag_truncator_test.go Outdated

Comment thread pkg/kv/kvserver/kvstorage/wag_truncator_test.go

iskettaneh requested a review from pav-kv April 8, 2026 21:03

iskettaneh and others added 2 commits April 8, 2026 20:20

iskettaneh force-pushed the rse_truncate_2 branch from 8108b23 to f121734 Compare April 9, 2026 00:24

pav-kv approved these changes Apr 9, 2026

View reviewed changes

trunk-io Bot merged commit 943fc6b into cockroachdb:master Apr 9, 2026
37 of 38 checks passed

celeste-cockroachdb Bot added the target-release-26.3.0 label Apr 9, 2026

Uh oh!

Conversation

iskettaneh commented Apr 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trunk-io Bot commented Apr 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Apr 4, 2026

Uh oh!

Uh oh!

pav-kv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

iskettaneh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iskettaneh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iskettaneh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pav-kv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iskettaneh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iskettaneh commented Apr 4, 2026 •

edited

Loading

trunk-io Bot commented Apr 4, 2026 •

edited

Loading