CNDB-13693: Make SAI's view referenceable to simplify locking query view #1700

michaeljmarshall · 2025-04-17T22:21:04Z

What is the issue

Fixes: https://github.com/riptano/cndb/issues/13693

What does this PR fix and why was it fixed

The new design is to:

Make sai View reference-able so that it holds references to the underlying SSTableIndexs. When the view is released the final time, it releases the indexes. This moves all of the complexity of grabbing references to sstable update time and out of the query path, which seems like a generally good improvement.
Observe that SSTableIndex holds a reference to its associated sstable reader.

Note also that we need to create the memtable index on deletions in order to make the index-first solution work.

Also note that sstables indexes are added before they are removed on flush, so this change in design is safe for flush.

github-actions · 2025-04-17T22:21:24Z

src/java/org/apache/cassandra/index/sai/IndexContext.java

src/java/org/apache/cassandra/index/sai/view/IndexViewManager.java

src/java/org/apache/cassandra/index/sai/plan/QueryView.java

jasonstack · 2025-04-21T05:34:14Z

I will review it tmr.

Update: I won't be able to review it on Tuesday. Will do it within this week.

michaeljmarshall · 2025-04-23T23:07:45Z

src/java/org/apache/cassandra/index/sai/disk/vector/CassandraOnHeapGraph.java

+                    if (matcher.apply(cv))
+                    {
+                        // We can exit now because we won't find a better candidate
+                        var candidate = new PqInfo(searcher.getPQ(), searcher.containsUnitVectors(), segment.metadata.numRows);


TODO: make sure the PQ is safe to use after releasing the view.

how about adding a SSTableIndex in PqInfo and release it when PqInfo is done

jasonstack · 2025-04-24T05:50:32Z

test/unit/org/apache/cassandra/index/SecondaryIndexManagerTest.java

        ((StorageAttachedIndex) cfs.getIndexManager().getIndex(indexMetadata)).getIndexContext().prepareSSTablesForRebuild(sstables);
        assertEmpty(execute("SELECT * FROM %s WHERE a=1"));
        assertEmpty(execute("SELECT * FROM %s WHERE c=1"));



This test testIndexRebuildWhenAddingSStableViaRemoteReload was added to test legacy writer behavior that built index on writer. Now writer only builds index on flush, not reload.

During sstable reload, reloaded sstable should have index attached (compaction task would abort if failed to build index; if initial index build is not completed, index is not-queryable, it's fine); unless it's added as shallow sstable.

this test can be removed if you prefer

src/java/org/apache/cassandra/index/sai/view/View.java

src/java/org/apache/cassandra/index/sai/view/IndexViewManager.java

jasonstack · 2025-04-24T07:16:51Z

src/java/org/apache/cassandra/index/sai/plan/QueryView.java

+
+                var sstableReaders = new ArrayList<SSTableReader>(saiView.size());
+                // These are already referenced because they are referenced by the same view we just referenced.
+                // TODO review saiView.match() method for boolean predicates.


there is an issue about boolean predicate support?

I'll create it. The point is that we have a data structure in the view to efficiently get the indexes that might satisfy the predicate. Without using match, we're just searching all indexes.

src/java/org/apache/cassandra/index/sai/plan/QueryView.java

src/java/org/apache/cassandra/index/sai/view/IndexViewManager.java

pkolaczk

This is a very nice simplification and this avoids all the looping with matching the indexes to sstables. However I have a concern if we don't miss any data in a very narrow race window when flush happens.

src/java/org/apache/cassandra/index/sai/IndexContext.java

src/java/org/apache/cassandra/index/sai/view/View.java

src/java/org/apache/cassandra/index/sai/plan/QueryView.java

…onstructor

jasonstack

Left one comment on potential race with shallow sstable, the rest LGTM

jasonstack · 2025-05-06T05:50:31Z

src/java/org/apache/cassandra/index/sai/view/IndexViewManager.java

            newViewIndexes.clear();

+            // Throw after releasing already referenced indexes
+            if (iterations++ > 1000)


nit: would be nice have a config property

I don't think it is likely that we'll hit this unless there is a bug in the logic. We have a similar ceiling at 1000 for the chunk cache:

cassandra/src/java/org/apache/cassandra/cache/ChunkCache.java

Lines 654 to 662 in 62e1537

if (buf == null && ++spin == 1000)

{

String msg = String.format("Could not acquire a reference to for %s after 1000 attempts. " +

"This is likely due to the chunk cache being too small for the " +

"number of concurrently running requests.", chunkKey);

throw new RuntimeException(msg);

// Note: this might also be caused by reference counting errors, especially double release of

// chunks.

}

This reverts commit d6dcf2f.

sonarqubecloud · 2025-05-06T16:41:07Z

Quality Gate passed

Issues
9 New issues
0 Accepted issues

Measures
0 Security Hotspots
80.3% Coverage on New Code
5.3% Duplication on New Code

See analysis details on SonarQube Cloud

cassci-bot · 2025-05-06T18:48:27Z

❌ Build ds-cassandra-pr-gate/PR-1700 rejected by Butler

2 new test failure(s) in 10 builds
See build details here

Found 2 new test failures

Test	Explanation	Branch history	Upstream history
...d.t.s.VectorDistributedTest.rangeRestrictedTest	regression	🔴🔵🔵🔵🔵🔵🔵	🔵🔵🔵🔵🔵🔵🔵
o.a.c.u.b.BinLogTest.testTruncationReleasesLogS...	regression	🔴🔵🔴🔵🔴🔵🔴	🔵🔵🔵🔵🔵🔵🔵

Found 22 known test failures

…iew (#1700) Fixes: riptano/cndb#13693 The new design is to: 1. Make sai `View` reference-able so that it holds references to the underlying `SSTableIndex`s. When the `view` is released the final time, it releases the indexes. This moves all of the complexity of grabbing references to sstable update time and out of the query path, which seems like a generally good improvement. 2. Observe that `SSTableIndex` holds a reference to its associated sstable reader. Note also that we need to create the memtable index on deletions in order to make the index-first solution work. Also note that sstables indexes are added before they are removed on flush, so this change in design is safe for flush. (cherry picked from commit 8f31782)

…iew (#1700) Fixes: riptano/cndb#13693 The new design is to: 1. Make sai `View` reference-able so that it holds references to the underlying `SSTableIndex`s. When the `view` is released the final time, it releases the indexes. This moves all of the complexity of grabbing references to sstable update time and out of the query path, which seems like a generally good improvement. 2. Observe that `SSTableIndex` holds a reference to its associated sstable reader. Note also that we need to create the memtable index on deletions in order to make the index-first solution work. Also note that sstables indexes are added before they are removed on flush, so this change in design is safe for flush.

CNDB-13693: Make SAI's view referenceable to simplify locking query view

4aa5552

Initialize SAI memtable index on (range) partition delete

67c556f

eolivelli reviewed Apr 18, 2025

View reviewed changes

src/java/org/apache/cassandra/index/sai/IndexContext.java Show resolved Hide resolved

eolivelli reviewed Apr 18, 2025

View reviewed changes

src/java/org/apache/cassandra/index/sai/view/IndexViewManager.java Outdated Show resolved Hide resolved

eolivelli reviewed Apr 18, 2025

View reviewed changes

src/java/org/apache/cassandra/index/sai/plan/QueryView.java Outdated Show resolved Hide resolved

Address review feedback

004949c

michaeljmarshall requested review from jasonstack and pkolaczk April 18, 2025 14:32

Fix compilation

3eb8761

michaeljmarshall added 2 commits April 23, 2025 16:46

Cleanup some old logic; fix possibly leaked view

059e5c6

Fix attemt to read from unreferenced view index

05ad2a7

michaeljmarshall commented Apr 23, 2025

View reviewed changes

Merge remote-tracking branch 'datastax/main' into cndb-13693

c2dd650

jasonstack reviewed Apr 24, 2025

View reviewed changes

src/java/org/apache/cassandra/index/sai/view/View.java Outdated Show resolved Hide resolved

jasonstack reviewed Apr 24, 2025

View reviewed changes

src/java/org/apache/cassandra/index/sai/view/IndexViewManager.java Outdated Show resolved Hide resolved

jasonstack reviewed Apr 24, 2025

View reviewed changes

src/java/org/apache/cassandra/index/sai/plan/QueryView.java Show resolved Hide resolved

jasonstack reviewed Apr 24, 2025

View reviewed changes

src/java/org/apache/cassandra/index/sai/view/IndexViewManager.java Outdated Show resolved Hide resolved

pkolaczk reviewed Apr 24, 2025

View reviewed changes

src/java/org/apache/cassandra/index/sai/IndexContext.java Outdated Show resolved Hide resolved

src/java/org/apache/cassandra/index/sai/view/View.java Outdated Show resolved Hide resolved

src/java/org/apache/cassandra/index/sai/plan/QueryView.java Outdated Show resolved Hide resolved

michaeljmarshall added 4 commits April 24, 2025 13:05

Remove sstables from View; minor cleanup from pr review

9af8e2f

Make getReferencedView terminate after a deadline

a02fc92

Simplify IndexViewManager::update by moving referencing out of View c…

6cc7105

…onstructor

Merge remote-tracking branch 'datastax/main' into cndb-13693

577ab01

jasonstack reviewed Apr 25, 2025

View reviewed changes

michaeljmarshall added 2 commits May 5, 2025 09:02

Merge remote-tracking branch 'datastax/main' into cndb-13693

7c53e1e

Add getMemtableIndex back (not sure if it will stay longterm)

d6dcf2f

jasonstack approved these changes May 6, 2025

View reviewed changes

Fix IndexViewManager#prepareSSTablesForRebuild logic

9546648

jasonstack approved these changes May 6, 2025

View reviewed changes

michaeljmarshall added 2 commits May 6, 2025 10:30

Update bm25 test to use getLiveMemtables()

52074d3

Revert "Add getMemtableIndex back (not sure if it will stay longterm)"

d1b32e9

This reverts commit d6dcf2f.

Formatting tweaks

43e7b2f

michaeljmarshall merged commit 8f31782 into main May 6, 2025
6 of 71 checks passed

michaeljmarshall deleted the cndb-13693 branch May 6, 2025 17:58

driftx mentioned this pull request Jun 11, 2025

CNDB-13693: Make SAI's view referenceable to simplify locking query v… #1803

Merged

michaeljmarshall mentioned this pull request Oct 2, 2025

CNDB-15487: Add queryability logic to View to prevent race to discover non-queryability #2029

Open

	if (buf == null && ++spin == 1000)
	{
	String msg = String.format("Could not acquire a reference to for %s after 1000 attempts. " +
	"This is likely due to the chunk cache being too small for the " +
	"number of concurrently running requests.", chunkKey);
	throw new RuntimeException(msg);
	// Note: this might also be caused by reference counting errors, especially double release of
	// chunks.
	}

CNDB-13693: Make SAI's view referenceable to simplify locking query view #1700

CNDB-13693: Make SAI's view referenceable to simplify locking query view #1700

Uh oh!

Conversation

michaeljmarshall commented Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the issue

What does this PR fix and why was it fixed

Uh oh!

github-actions bot commented Apr 17, 2025

Checklist before you submit for review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jasonstack commented Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pkolaczk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jasonstack left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented May 6, 2025

Quality Gate passed

Uh oh!

Uh oh!

cassci-bot commented May 6, 2025

❌ Build ds-cassandra-pr-gate/PR-1700 rejected by Butler

Found 2 new test failures

Found 22 known test failures

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

michaeljmarshall commented Apr 17, 2025 •

edited

Loading

jasonstack commented Apr 21, 2025 •

edited

Loading