Skip to content

CASSANDRA-21216/CASSANDRA-21260: Clear savedBuffer and savedNextKey in BTree.FastBuilder.reset() #4792

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
andresbeckruiz wants to merge 1 commit into
base: cassandra-5.0
Choose a base branch
from
+372 −78
Open

CASSANDRA-21216/CASSANDRA-21260: Clear savedBuffer and savedNextKey in BTree.FastBuilder.reset() #4792

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
5.0.9
* BTree.FastBuilder.reset() fails to clear savedBuffer and savedNextKey, causing ClassCastException and SSTable header corruption during schema disagreement (CASSANDRA-21216, CASSANDRA-21260)
* Use estimated compressed size for tables to check if there is enough free space for a compaction (CASSANDRA-21245)
* Fix failing select on system_views.settings for non-string keys (CASSANDRA-21348)
* Ensure SAI sends range tombstones to the coordinator for queries on static columns (CASSANDRA-21332)
Expand Down
133 changes: 55 additions & 78 deletions src/java/org/apache/cassandra/utils/btree/BTree.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3081,7 +3081,7 @@ Object[] drain()
* was constructed from for the contents of {@code buffer}.
* <p>
* For {@link FastBuilder} these are mostly the same, so they are fetched from a global cache and
* resized accordingly, but for {@link AbstractUpdater} we maintain a buffer of sizes.
* resized accordingly, but for {@link Updater} we maintain a buffer of sizes.
*/
int setDrainSizeMap(Object[] original, int keysInOriginal, Object[] branch, int keysInBranch)
{
Expand Down Expand Up @@ -3110,7 +3110,7 @@ int setDrainSizeMap(Object[] original, int keysInOriginal, Object[] branch, int
* was constructed from for the contents of {@code savedBuffer}.
* <p>
* For {@link FastBuilder} these are always the same size, so they are fetched from a global cache,
* but for {@link AbstractUpdater} we maintain a buffer of sizes.
* but for {@link Updater} we maintain a buffer of sizes.
*
* @return the size of {@code branch}
*/
Expand Down Expand Up @@ -3141,7 +3141,7 @@ int setOverflowSizeMap(Object[] branch, int keys)
* was constructed from the contents of both {@code savedBuffer} and {@code buffer}
* <p>
* For {@link FastBuilder} these are mostly the same size, so they are fetched from a global cache
* and only the last items updated, but for {@link AbstractUpdater} we maintain a buffer of sizes.
* and only the last items updated, but for {@link Updater} we maintain a buffer of sizes.
*/
void setRedistributedSizeMap(Object[] branch, int steal)
{
Expand Down Expand Up @@ -3269,11 +3269,57 @@ final LeafBuilder leaf()

/**
* Clear any references we might still retain, to avoid holding onto memory.
* <p>
* While this method is not strictly necessary, it exists to
* ensure the implementing classes are aware they must handle it.
*/
abstract void reset();
void reset()
{
leaf().count = 0;
clearLeafBuffer(leaf().buffer);
if (leaf().savedBuffer != null)
clearLeafBuffer(leaf().savedBuffer);
leaf().savedNextKey = null;
BranchBuilder branch = leaf().parent;
while (branch != null && branch.inUse)
{
branch.count = 0;
clearBranchBuffer(branch.buffer);
if (branch.savedBuffer != null)
clearBranchBuffer(branch.savedBuffer);
branch.savedNextKey = null;
branch.inUse = false;
branch = branch.parent;
}
}

/**
* Clear the contents of a leaf buffer, aborting once we encounter a null entry
* to save time on small trees
*/
private void clearLeafBuffer(Object[] array)
{
if (array[0] == null)
return;
// find first null entry; loop from beginning, to amortise cost over size of working set
int i = 1;
while (i < array.length && array[i] != null)
++i;
Arrays.fill(array, 0, i, null);
}

/**
* Clear the contents of a branch buffer, aborting once we encounter a null entry
* to save time on small trees
*/
private void clearBranchBuffer(Object[] array)
{
if (array[0] == null && array[MAX_KEYS] == null)
return;
// find first null entry; loop from beginning, to amortise cost over size of working set
int i = 1;
while (i < MAX_KEYS && array[i] != null)
++i;
Arrays.fill(array, 0, i, null);
Arrays.fill(array, MAX_KEYS, MAX_KEYS + i + 1, null);
}
}

/**
Expand Down Expand Up @@ -3325,21 +3371,6 @@ public void close()
}
}

@Override
void reset()
{
Arrays.fill(leaf().buffer, null);
leaf().count = 0;
BranchBuilder branch = leaf().parent;
while (branch != null && branch.inUse)
{
Arrays.fill(branch.buffer, null);
branch.count = 0;
branch.inUse = false;
branch = branch.parent;
}
}

public boolean validateEmpty()
{
LeafOrBranchBuilder cur = leaf();
Expand All @@ -3366,60 +3397,6 @@ private static boolean hasOnlyNulls(Object[] buffer)
}
}

private static abstract class AbstractUpdater extends AbstractFastBuilder implements AutoCloseable
{
void reset()
{
assert leaf().count == 0;
clearLeafBuffer(leaf().buffer);
if (leaf().savedBuffer != null)
Arrays.fill(leaf().savedBuffer, null);

BranchBuilder branch = leaf().parent;
while (branch != null && branch.inUse)
{
assert branch.count == 0;
clearBranchBuffer(branch.buffer);
if (branch.savedBuffer != null && branch.savedBuffer[0] != null)
Arrays.fill(branch.savedBuffer, null); // by definition full, if non-empty
branch.inUse = false;
branch = branch.parent;
}
}

/**
* Clear the contents of a branch buffer, aborting once we encounter a null entry
* to save time on small trees
*/
private void clearLeafBuffer(Object[] array)
{
if (array[0] == null)
return;
// find first null entry; loop from beginning, to amortise cost over size of working set
int i = 1;
while (i < array.length && array[i] != null)
++i;
Arrays.fill(array, 0, i, null);
}

/**
* Clear the contents of a branch buffer, aborting once we encounter a null entry
* to save time on small trees
*/
private void clearBranchBuffer(Object[] array)
{
if (array[0] == null)
return;

// find first null entry; loop from beginning, to amortise cost over size of working set
int i = 1;
while (i < MAX_KEYS && array[i] != null)
++i;
Arrays.fill(array, 0, i, null);
Arrays.fill(array, MAX_KEYS, MAX_KEYS + i + 1, null);
}
}

/**
* A pooled object for modifying an existing tree with a new (typically smaller) tree.
* <p>
Expand All @@ -3434,7 +3411,7 @@ private void clearBranchBuffer(Object[] array)
* Searches within both trees to accelerate the process of modification, instead of performing a simple
* iteration over the new tree.
*/
private static class Updater<Compare, Existing extends Compare, Insert extends Compare> extends AbstractUpdater implements AutoCloseable
private static class Updater<Compare, Existing extends Compare, Insert extends Compare> extends AbstractFastBuilder implements AutoCloseable
{
static final TinyThreadLocalPool<Updater> POOL = new TinyThreadLocalPool<>();
TinyThreadLocalPool.TinyPool<Updater> pool;
Expand Down Expand Up @@ -3647,7 +3624,7 @@ static int searchResultToComparison(int searchResult)
* <p>
* The approach taken here hopefully balances simplicity, garbage generation and execution time.
*/
private static abstract class AbstractTransformer<I, O> extends AbstractUpdater implements AutoCloseable
private static abstract class AbstractTransformer<I, O> extends AbstractFastBuilder implements AutoCloseable
{
/**
* An iterator over the tree we are updating
Expand Down
Loading