diff --git a/CHANGES.txt b/CHANGES.txt index d6f4a08610..27a812a11a 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -8,6 +8,7 @@ Merged from 4.1: * Add Paxos v2 option and informatin in cassandra.yaml (CASSANDRA-21316) Merged from 4.0: + * 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) diff --git a/src/java/org/apache/cassandra/db/rows/BTreeRow.java b/src/java/org/apache/cassandra/db/rows/BTreeRow.java index 52f0639e8e..a0cf179237 100644 --- a/src/java/org/apache/cassandra/db/rows/BTreeRow.java +++ b/src/java/org/apache/cassandra/db/rows/BTreeRow.java @@ -605,10 +605,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); } } diff --git a/src/java/org/apache/cassandra/db/rows/ColumnData.java b/src/java/org/apache/cassandra/db/rows/ColumnData.java index b9f19dc07f..5740af096c 100644 --- a/src/java/org/apache/cassandra/db/rows/ColumnData.java +++ b/src/java/org/apache/cassandra/db/rows/ColumnData.java @@ -179,7 +179,15 @@ public abstract class ColumnData implements IMeasurableMemory 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); } diff --git a/test/unit/org/apache/cassandra/db/partitions/MemtableNegativeReleasedCQLReproTest.java b/test/unit/org/apache/cassandra/db/partitions/MemtableNegativeReleasedCQLReproTest.java new file mode 100644 index 0000000000..ab8aef6355 --- /dev/null +++ b/test/unit/org/apache/cassandra/db/partitions/MemtableNegativeReleasedCQLReproTest.java @@ -0,0 +1,151 @@ +/* + * 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.List; +import java.util.Random; + +import com.google.common.collect.ImmutableList; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameter; +import org.junit.runners.Parameterized.Parameters; + +import org.apache.cassandra.cql3.CQLTester; +import org.apache.cassandra.db.ColumnFamilyStore; +import org.apache.cassandra.db.memtable.Memtable; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * CQL-driven regression test for CASSANDRA-21469 ("TrieMemtable MemtableReclaimMemory AssertionError: + * Negative released in MemtablePool$SubPool"). + *

+ * It drives the same overwrite/delete churn through the public CQL write path + * (with explicit {@code USING TIMESTAMP} so reconciliation is deterministic), then inspects the live + * memtable's on-heap ownership after every statement and forces a flush to exercise the real discard/release path. + *

+ * Root cause: the memtable on-heap accounting in {@link BTreePartitionUpdater} + * over-subtracts when an update's cells are SHADOWED by an existing, + * newer row/partition deletion. The shadowed cells' heap overhead is subtracted from {@code owns} + * even though it was never added, so under repair-style churn (re-writing cells into a partition that + * already carries a newer tombstone) {@code owns} drifts below zero and the next flush trips + * {@code AssertionError: Negative released} in {@code MemtablePool$SubPool.released} during discard. + *

+ * 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 + * ({@code MemtablePool$SubPool.released <- SubAllocator.releaseAll}) and reproduces the exact + * production stack when {@code owns} has gone negative. + *

+ * Note on minimisation: the {@code PartitionUpdate}-level sibling reproduces with a single shadowed + * update, but the CQL path first clones the incoming row into the memtable allocator and only then + * merges, so an isolated shadowed statement nets close to zero. The drift only becomes observable + * across mixed churn with non-monotonic timestamps — which is exactly the repair re-streaming + * workload that hit this in the field — hence the fixed-seed churn below rather than a one-liner. + */ +@RunWith(Parameterized.class) +public class MemtableNegativeReleasedCQLReproTest extends CQLTester +{ + @Parameter + public String memtableClass; + + @Parameters(name = "memtable={0}") + public static List parameters() + { + return ImmutableList.of("skiplist", "skiplist_sharded", "trie"); + } + + /** + * 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() + { + for (int seed = 0; seed < 50; seed++) + { + createTable("CREATE TABLE %s (pk int, ck int, r1 int, c2 set, PRIMARY KEY (pk, ck)) " + + "WITH memtable = '" + memtableClass + "'"); + 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 (memtable=%s, " + + "seed=%d, mutation=%d) — the memtable reported releasing more on-heap memory than it " + + "allocated (CASSANDRA-21469)", memtableClass, 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.getMemoryUsage(memtable).ownsOnHeap; + } + + /** + * A varying mutation of the same partition: live overwrite, row tombstone, or partition deletion. + */ + private void mutateSamePartition(Random rnd) + { + // 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; + } + } + } +}