natas_hw
/
cassandra
mirror of https://github.com/apache/cassandra
0
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Fix negative memtable allocator ownership when an update is shadowed by an existing row deletion 

When BTreeRow.merge reconciles an update into a row whose existing deletion
shadows the update's cells, it filtered the update (incoming) side of the merge
with Reconciler.retain, which records the removal via
PostReconciliationFunction.delete. On the memtable write path that subtracts the
shadowed cells' on-heap size from the allocator's ownership even though that
incoming data was never allocated to the memtable. Under overwrite/delete churn
that re-applies cells already covered by a newer row/partition deletion (e.g.
repair re-streaming), the allocator's "owns" counter drifts negative and the next
flush trips "AssertionError: Negative released" in MemtablePool$SubPool.released
via MemtableAllocator$SubAllocator.releaseAll during discard.

Filter the update (incoming) side with a non-recording variant,
Reconciler.removeShadowed, and keep retain() for the existing side,
whose data the memtable already owns. The filtered result is identical; only the
erroneous accounting notification is dropped. This mirrors the already-correct
complex-column path in ColumnData.merge.

patch by Stefan Miklosovic; reviewed by Dmitry Konstantinov for CASSANDRA-21469

Assisted-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Stefan Miklosovic 2026-06-16 19:18:27 +02:00
parent 41fd7c3e22
commit d30ac083b8
No known key found for this signature in database
GPG Key ID: 32F35CB2F546D93E
4 changed files with 151 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
CHANGES.txt
View File

@ -1,4 +1,5 @@
4.0.21
* Fix negative memtable allocator ownership when an update is shadowed by an existing row deletion (CASSANDRA-21469)
* Consider first token of SSTable when calculating SSTable intersection in LeveledScanner (CASSANDRA-21369)
* Remove inFlightEcho entry on ECHO_REQ failure (CASSANDRA-21428)
* Validate snapshot names (CASSANDRA-21389)

8
src/java/org/apache/cassandra/db/rows/BTreeRow.java
View File

@ -578,10 +578,16 @@ public class BTreeRow extends AbstractRow
{
if (rowDeletion == existing.deletion())
{
updateBtree = BTree.transformAndFilter(updateBtree, reconciler::retain);
// The existing row's deletion shadows part of the update. Filter those cells out of
// the UPDATE (incoming) side, but do NOT record their removal: that data was never
// owned by the memtable, so accounting its removal would drive the allocator's
// ownership negative and crash the next flush (CASSANDRA-21469).
updateBtree = BTree.transformAndFilter(updateBtree, reconciler::removeShadowed);
}
else
{
// The update's deletion shadows part of the existing row. Those cells ARE owned by
// the memtable, so record their removal via retain().
existingBtree = BTree.transformAndFilter(existingBtree, reconciler::retain);
}
}

10
src/java/org/apache/cassandra/db/rows/ColumnData.java
View File

@ -178,7 +178,15 @@ public abstract class ColumnData
return removeShadowed(existing, postReconcile);
}
private ColumnData removeShadowed(ColumnData existing)
/**
* Like {@link #retain} but does NOT notify the {@link PostReconciliationFunction} of removed
* data. Use this e.g. when filtering shadowed cells out of the UPDATE (incoming) side of a merge:
* that data was never allocated to / owned by the memtable, so recording its removal would
* make the memtable allocator under-count what it owns and eventually report a negative
* release at flush (CASSANDRA-21469). Recording removals (via {@link #retain}) is only correct
* for the EXISTING side, whose data the memtable already owns.
*/
public ColumnData removeShadowed(ColumnData existing)
{
return removeShadowed(existing, ColumnData.noOp);
}

134
test/unit/org/apache/cassandra/db/partitions/MemtableNegativeReleasedCQLReproTest.java Normal file
View File

@ -0,0 +1,134 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.apache.cassandra.db.partitions;
import java.util.Random;
import org.junit.Test;
import org.apache.cassandra.cql3.CQLTester;
import org.apache.cassandra.db.ColumnFamilyStore;
import org.apache.cassandra.db.Memtable;
import static org.assertj.core.api.Assertions.assertThat;
/**
* CQL-driven regression test for CASSANDRA-21469 ("MemtableReclaimMemory AssertionError:
* Negative released in MemtablePool$SubPool").
* <p>
* It drives overwrite/delete churn through the public CQL write path (with explicit
* {@code USING TIMESTAMP} so reconciliation is deterministic), inspects the live memtable's
* on-heap ownership after every statement, and forces a flush to exercise the real discard/release path.
* <p>
* Root cause: when {@link org.apache.cassandra.db.rows.BTreeRow}{@code .merge} reconciles an update into a
* row whose existing deletion shadows the update's cells, it filtered the update (incoming) side of the
* merge with {@code Reconciler.retain}, which records the removal via {@code PostReconciliationFunction.delete}.
* On the memtable write path that subtracts the shadowed cells' on-heap size from the allocator's ownership
* even though that incoming data was never allocated to the memtable. Under overwrite/delete churn that
* re-applies cells already covered by a newer row/partition deletion (e.g. repair re-streaming), the
* allocator's {@code owns} counter drifts below zero and the next flush trips
* {@code AssertionError: Negative released} in {@code MemtablePool$SubPool.released} via
* {@code MemtableAllocator$SubAllocator.releaseAll} during discard.
* <p>
* The test asserts the same invariant the production code asserts at flush
* ({@code memtable on-heap owns >= 0}) and then flushes, which runs the real discard path and reproduces
* the exact production stack when {@code owns} has gone negative.
* <p>
* Note on minimisation: a single shadowed update nets close to zero because the CQL path first clones the
* incoming row into the memtable allocator and only then merges. The drift only becomes observable across
* mixed churn with non-monotonic timestamps — exactly the repair re-streaming workload that hit this in the
* field — hence the fixed-seed churn below rather than a one-liner.
*/
public class MemtableNegativeReleasedCQLReproTest extends CQLTester
{
/**
* Deterministic fixed-seed churn of overwrites / row tombstones / partition deletions against a
* single partition (what repair re-streaming does). On buggy code this drives the memtable's
* on-heap ownership negative; the per-statement assertion catches the drift and the trailing flush
* reproduces the exact production discard crash.
*/
@Test
public void repeatedOverwriteAndDeleteChurnKeepsOwnsNonNegative() throws Throwable
{
for (int seed = 0; seed < 50; seed++)
{
createTable("CREATE TABLE %s (pk int, ck int, r1 int, c2 set<int>, PRIMARY KEY (pk, ck))");
ColumnFamilyStore cfs = getCurrentColumnFamilyStore();
cfs.disableAutoCompaction();
Random rnd = new Random(seed);
for (int i = 0; i < 40; i++)
{
mutateSamePartition(rnd);
assertThat(currentOnHeapOwns(cfs))
.as("memtable on-heap owns went NEGATIVE during overwrite/delete churn (seed=%d, mutation=%d) " +
"— the memtable reported releasing more on-heap memory than it allocated (CASSANDRA-21469)",
seed, i)
.isGreaterThanOrEqualTo(0L);
}
// The real crash path: flush discards the memtable, running releaseAll() -> SubPool.released(owns),
// which throws AssertionError("Negative released: ...") under -ea if owns ever went negative.
flush();
}
}
private long currentOnHeapOwns(ColumnFamilyStore cfs)
{
Memtable memtable = cfs.getTracker().getView().getCurrentMemtable();
return memtable.getAllocator().onHeap().owns();
}
/**
* A varying mutation of the same partition: live overwrite, row tombstone, or partition deletion.
*/
private void mutateSamePartition(Random rnd) throws Throwable
{
// RANDOM (non-monotonic) timestamp is essential: it makes reconciliation sometimes keep the
// existing cell and sometimes shadow/replace it, producing the merges that over-subtract.
long ts = 1000 + rnd.nextInt(3000);
switch (rnd.nextInt(6))
{
case 0: // whole-partition deletion
execute("DELETE FROM %s USING TIMESTAMP " + ts + " WHERE pk = 0");
break;
case 1: // whole-row tombstone
execute("DELETE FROM %s USING TIMESTAMP " + ts + " WHERE pk = 0 AND ck = 0");
break;
default: // live overwrite of varying shape
{
StringBuilder set = new StringBuilder("r1 = ").append(rnd.nextInt());
int cells = rnd.nextInt(5);
StringBuilder elements = new StringBuilder();
for (int k = 0; k < cells; k++)
{
if (elements.length() > 0)
elements.append(", ");
elements.append(1000 + rnd.nextInt(8));
}
if (cells > 0)
{
if (rnd.nextInt(3) == 0)
set.append(", c2 = {").append(elements).append('}'); // full overwrite -> complex deletion
else
set.append(", c2 = c2 + {").append(elements).append('}'); // append cells, no complex deletion
}
execute("UPDATE %s USING TIMESTAMP " + ts + " SET " + set + " WHERE pk = 0 AND ck = 0");
break;
}
}
}
}