kvserver/rangefeed: checkpoint events can cause OOMs

[nicktrav]

Describe the problem

OOMs were observed during a large scale test, specifically when testing changefeed / rangefeed performance.

Heap profiles point the finger at checkpoint events. Retained size and number of objects is far beyond what would be expected if memory budgets were being respected:

inuse_space:

inuse_objects:

Environment:

• CockroachDB version V23.2.0-ALPHA.6-DEV
• Server OS: Linux.

Additional context

More context here (internal).

Debugging artifacts can be found here (internal).

Jira issue: CRDB-33387

[blathers-crl]

cc @cockroachdb/replication

[nicktrav]

cc @cockroachdb/cdc

[erikgrinaker]

This seems legit. The changefeed processors struggled to keep up for whatever reason, and we don't subject checkpoints to memory budgets. The checkpoint events were likely sitting around in the registration channel buffers, which have a 4096-event size per replica (so 614M events with 150k replicas, which can eat up to 40 GB memory if they're all filled up -- which is very unlikely, but gives an upper bound).

cockroach/pkg/kv/kvserver/rangefeed/scheduled_processor.go

Lines 772 to 773 in ab00ae9

	event := p.newCheckpointEvent()
	p.reg.PublishToOverlapping(ctx, all, event, nil)

cockroach/pkg/kv/kvserver/rangefeed/processor.go

Lines 953 to 967 in 9640975

	// calculateDateEventSize returns estimated size of the event that contain actual
	// data. We only account for logical ops and sst's. Those events come from raft
	// and are budgeted. Other events come from processor jobs and update timestamps
	// we don't take them into account as they are supposed to be small and to avoid
	// complexity of having multiple producers getting from budget.
	func calculateDateEventSize(e event) int64 {
	var size int64
	for _, op := range e.ops {
	size += int64(op.Size())
	}
	if e.sst != nil {
	size += int64(len(e.sst.data))
	}
	return size
	}

[miretskiy]

I don't know if this is a release blocker -- it's not a regression. @erikgrinaker ?

[erikgrinaker]

Yeah, not a blocker. It has always been like this, and it isn't clear that a fix would be safe this late in the cycle.

[blathers-crl]

cc @cockroachdb/cdc

[wenyihu6]

Each rangefeed processor has an event buffering channel eventC. These events could point to large underlying data structure, and keeping them buffered in the channel prevents the underlying data from being garbage collected. To mitigate OOMs, we previously introduced memory budget to limit event buffering to the eventC channel.

cockroach/pkg/kv/kvserver/rangefeed/scheduled_processor.go

Line 64 in 690c67f

eventC chan *event

This is how the memory accounting model works currently:

Before sending an event to the eventC channel, it calculatesDataSize based on the event to predict how much memory an event would take. It tries to allocate memory from the budget feed. The event then holds onto the same memory alloc and passed to registrations as part of a shared event when publishing the events. As the registry publishes the event to registrations, it will create a new rangefeed event (e.x.https://github.com/cockroachdb/cockroach/blob/aa58da93a7fa83bbf905c2b096b9f1954d35d402/pkg/kv/kvserver/rangefeed/scheduled_processor.go#L748-L757) based on event c and a shared event holding the memory alloc. It then increases the reference count of the memory alloc. As registration publishes the event to its stream, it decreases the reference count of the memory alloc

cockroach/pkg/kv/kvserver/rangefeed/registry.go

Line 343 in 6fcbdfc

nextEvent.alloc.Release(ctx)

. The memory is then returned back to the budget feed when reference count becomes zero

cockroach/pkg/kv/kvserver/rangefeed/budget.go

Lines 263 to 264 in 3df5f2c

	if a != nil && atomic.AddInt32(&a.refCount, -1) == 0 {
	a.feed.returnAllocation(ctx, a.size)

.

However, the current memory accounting did not have a good way to predict the memory footprint of events. It uses calculateDateSize which 1. Does not account for memory of the new RangeFeedEvents that could be spawned (such as checkpoint events

cockroach/pkg/kv/kvserver/rangefeed/scheduled_processor.go

Line 806 in 6681efb

func (p *ScheduledProcessor) newCheckpointEvent() *kvpb.RangeFeedEvent {

) 2. inaccurately uses protobuf-encoded wire size for data (

cockroach/pkg/kv/kvserver/rangefeed/processor.go

Line 303 in 6681efb

size += int64(op.Size())

) 3. does not account for memory of the base structs.

The challenging part of this issue is we are not sure if new rangefeed events can be spawned before buffering event e to the eventC channel.

• Solution 1 (most precise, most complex): Ideally, we would want to allocate memory only when we know for sure that we are publishing a checkpoint event so that we are not over- or under-accounting. This is hard since once the processor sends the event to the eventC channel, it is committed to sending it unless we try to disconnect the scheduler. It cannot backoff and decide not to send it if it cannot acquire a memory budget.
• Solution 2: We can use the processor's resolved timestamp to predict if a checkpoint event will happen. But calling p.rts.ConsumeLogicalOp for memory accounting can change internal field unresolvedIntentQueue. To resolve this, we could make a deep copy of rts, but this is on a hot path and could introduce significant overhead. (Open pr: [wip] rangefeed: improve memory accounting for event queue #120001)
• Solution 3: We over-accounts at the beginning to account for everything that could have happened. If the memory turns out to be not needed, we de-allocates the memory right away to prevent registrations from holding onto unnecessary memory for too long. (Open pr: rangefeed: improve memory accounting for event queue #120550 ,rangefeed: add memory adjuster before publishing events #120348)
• Solution 4 (least precise, straightforward): We roughly estimate the memory and sacrifice some precision for simplicity. We are allowing small overhead of shared event structs and checkpoint events caused by logical ops to slip away from memory accounting. (Open pr: rangefeed: improve memory accounting for event queue #120347)

Currently, we are picking solution 4 for its simplicity and will revisit this topic if anything comes up.

More details for solution 4
Instead of accounting memory for both the current event and future rangefeed events, it will always pick the maximum memory between the two. It also allows additional shared event overhead (should be very small) and checkpoint events (caused by logical ops) (should be rare) to slip away.

• For logical ops, we always take current event memory. Current event memory is always larger if we assume no checkpoint events will be published for any logical ops. (same underlying data)
• For checkpoint events (ct and initRTS), we always take checkpoint rangefeed event. Future events are larger. (curr: 80, future: 152) (span points to the same underlying data)
• For sst events, we always take future rangefeed event (future base structs is larger than current base structs by 2) (same underlying data).
• For sync events, we always take current events. (current memory usage is larger as no future events will be created.

We got these data ^ from test benchmarks documented here #120582.