Skip to content

Commit 92577c2

Browse files
committed
sql: harden recent fix to deleteRange
In recently merged 59a28ec we fixed how we count "rows affected" by the deleteRangeNode, which was done by maintaining "cur row prefix" across BatchRequests. AI-generated code review pointed out that we can alias the memory of now-old BatchRequest while processing the response to the new one. Although I don't think it can lead to problems (since we shouldn't be modifying the BatchRequest's or BatchResponse's keys - which is verified via `GRPCTransportFactory` "race" variant), it seems prudent that we make a copy of the row prefix (among other benefits, this might allow for the old keys to be GCed sooner). Release note: None
1 parent 37243f5 commit 92577c2

File tree

1 file changed

+13
-0
lines changed

1 file changed

+13
-0
lines changed

pkg/sql/delete_range.go

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,7 @@ func (d *deleteRangeNode) startExec(params runParams) error {
111111

112112
ctx := params.ctx
113113
log.VEvent(ctx, 2, "fast delete: skipping scan")
114+
// TODO(yuzefovich): why are we making a copy of spans?
114115
spans := make([]roachpb.Span, len(d.spans))
115116
copy(spans, d.spans)
116117
if !d.autoCommitEnabled {
@@ -198,6 +199,18 @@ func (d *deleteRangeNode) deleteSpans(params runParams, b *kv.Batch, spans roach
198199
func (d *deleteRangeNode) processResults(
199200
results []kv.Result, resumeSpans []roachpb.Span,
200201
) (roachpb.Spans, error) {
202+
if !d.autoCommitEnabled {
203+
defer func() {
204+
// Make a copy of curRowPrefix to avoid referencing the memory from
205+
// the now-old BatchRequest.
206+
//
207+
// When auto-commit is enabled, we expect to see any resume spans,
208+
// so we won't need to access d.curRowPrefix later.
209+
curRowPrefix := make([]byte, len(d.curRowPrefix))
210+
copy(curRowPrefix, d.curRowPrefix)
211+
d.curRowPrefix = curRowPrefix
212+
}()
213+
}
201214
for _, r := range results {
202215
// TODO(yuzefovich): when the table has 1 column family, we don't need
203216
// to compare the key prefixes since each deleted key corresponds to a

0 commit comments

Comments
 (0)