diff --git a/CHANGES.txt b/CHANGES.txt index 617a414473..3b14a53880 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -6,6 +6,7 @@ * Add the ability to disable bulk loading of SSTables (CASSANDRA-18781) * Clean up obsolete functions and simplify cql_version handling in cqlsh (CASSANDRA-18787) Merged from 5.0: + * Fix resource cleanup after SAI query timeouts (CASSANDRA-19177) * Suppress CVE-2023-6481 (CASSANDRA-19184) Merged from 4.1: * Fix StackOverflowError on ALTER after many previous schema changes (CASSANDRA-19166) diff --git a/src/java/org/apache/cassandra/index/sai/disk/IndexSearchResultIterator.java b/src/java/org/apache/cassandra/index/sai/disk/IndexSearchResultIterator.java index 1433598cc8..2904bb2585 100644 --- a/src/java/org/apache/cassandra/index/sai/disk/IndexSearchResultIterator.java +++ b/src/java/org/apache/cassandra/index/sai/disk/IndexSearchResultIterator.java @@ -115,26 +115,12 @@ public class IndexSearchResultIterator extends KeyRangeIterator protected PrimaryKey computeNext() { - try - { - return union.hasNext() ? union.next() : endOfData(); - } - finally - { - context.checkpoint(); - } + return union.hasNext() ? union.next() : endOfData(); } protected void performSkipTo(PrimaryKey nextKey) { - try - { - union.skipTo(nextKey); - } - finally - { - context.checkpoint(); - } + union.skipTo(nextKey); } public void close() diff --git a/src/java/org/apache/cassandra/index/sai/disk/SSTableRowIdKeyRangeIterator.java b/src/java/org/apache/cassandra/index/sai/disk/SSTableRowIdKeyRangeIterator.java deleted file mode 100644 index 46bc4d54da..0000000000 --- a/src/java/org/apache/cassandra/index/sai/disk/SSTableRowIdKeyRangeIterator.java +++ /dev/null @@ -1,160 +0,0 @@ -/* - * 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.index.sai.disk; - -import java.io.IOException; -import java.lang.invoke.MethodHandles; -import java.util.Arrays; -import java.util.concurrent.TimeUnit; -import javax.annotation.concurrent.NotThreadSafe; - -import com.google.common.base.Stopwatch; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -import org.apache.cassandra.exceptions.QueryCancelledException; -import org.apache.cassandra.index.sai.QueryContext; -import org.apache.cassandra.index.sai.iterators.KeyRangeIterator; -import org.apache.cassandra.index.sai.postings.PostingList; -import org.apache.cassandra.index.sai.utils.PrimaryKey; -import org.apache.cassandra.io.util.FileUtils; -import org.apache.cassandra.utils.Throwables; - - -/** - * From sstable row id range iterator to primary key range iterator - */ -@NotThreadSafe -public class SSTableRowIdKeyRangeIterator extends KeyRangeIterator -{ - private static final Logger logger = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); - - private final Stopwatch timeToExhaust = Stopwatch.createStarted(); - private final QueryContext queryContext; - private final PrimaryKeyMap primaryKeyMap; - private final PostingList postingList; - - private boolean needsSkipping = false; - private PrimaryKey skipToToken = null; - - /** - * Create a direct PostingListRangeIterator where the underlying PostingList is materialised - * immediately so the posting list size can be used. - */ - private SSTableRowIdKeyRangeIterator(PrimaryKey min, - PrimaryKey max, - long count, - PrimaryKeyMap primaryKeyMap, - QueryContext queryContext, - PostingList postingList) - { - super(min, max, count); - - this.primaryKeyMap = primaryKeyMap; - this.queryContext = queryContext; - this.postingList = postingList; - } - - public static KeyRangeIterator create(PrimaryKeyMap primaryKeyMap, QueryContext queryContext, PostingList postingList) - { - if (postingList.size() <= 0) - return KeyRangeIterator.empty(); - - PrimaryKey min = primaryKeyMap.primaryKeyFromRowId(postingList.minimum()); - PrimaryKey max = primaryKeyMap.primaryKeyFromRowId(postingList.maximum()); - return new SSTableRowIdKeyRangeIterator(min, max, postingList.size(), primaryKeyMap, queryContext, postingList); - } - - @Override - protected void performSkipTo(PrimaryKey nextKey) - { - if (skipToToken != null && skipToToken.compareTo(nextKey) >= 0) - return; - - skipToToken = nextKey; - needsSkipping = true; - } - - @Override - protected PrimaryKey computeNext() - { - try - { - queryContext.checkpoint(); - - // just end the iterator if we don't have a postingList or current segment is skipped - if (exhausted()) - return endOfData(); - - long rowId = getNextRowId(); - if (rowId == PostingList.END_OF_STREAM) - return endOfData(); - - return primaryKeyMap.primaryKeyFromRowId(rowId); - } - catch (Throwable t) - { - //VSTODO We aren't tidying up resources here - if (!(t instanceof QueryCancelledException)) - logger.error("Unable to provide next token!", t); - - throw Throwables.cleaned(t); - } - } - - @Override - public void close() - { - if (logger.isTraceEnabled()) - { - final long exhaustedInMills = timeToExhaust.stop().elapsed(TimeUnit.MILLISECONDS); - logger.trace("PostinListRangeIterator exhausted after {} ms", exhaustedInMills); - } - - FileUtils.closeQuietly(Arrays.asList(postingList, primaryKeyMap)); - } - - private boolean exhausted() - { - return needsSkipping && skipToToken.compareTo(getMaximum()) > 0; - } - - /** - * reads the next sstable row ID from the underlying range iterator, potentially skipping to get there. - */ - private long getNextRowId() throws IOException - { - long sstableRowId; - if (needsSkipping) - { - long targetRowID = primaryKeyMap.rowIdFromPrimaryKey(skipToToken); - // skipToToken is larger than max token in token file - if (targetRowID < 0) - return PostingList.END_OF_STREAM; - - sstableRowId = postingList.advance(targetRowID); - needsSkipping = false; - } - else - { - sstableRowId = postingList.nextPosting(); - } - - return sstableRowId; - } -} diff --git a/src/java/org/apache/cassandra/index/sai/disk/v1/postings/PostingListRangeIterator.java b/src/java/org/apache/cassandra/index/sai/disk/v1/postings/PostingListRangeIterator.java index 3bab8092eb..c51cafcc74 100644 --- a/src/java/org/apache/cassandra/index/sai/disk/v1/postings/PostingListRangeIterator.java +++ b/src/java/org/apache/cassandra/index/sai/disk/v1/postings/PostingListRangeIterator.java @@ -118,6 +118,7 @@ public class PostingListRangeIterator extends KeyRangeIterator if (!(t instanceof QueryCancelledException)) logger.error(indexIdentifier.logMessage("Unable to provide next token!"), t); + FileUtils.closeQuietly(Arrays.asList(postingList, primaryKeyMap)); throw Throwables.cleaned(t); } } diff --git a/src/java/org/apache/cassandra/index/sai/plan/QueryController.java b/src/java/org/apache/cassandra/index/sai/plan/QueryController.java index ebd8fc20d0..f5c25d4b04 100644 --- a/src/java/org/apache/cassandra/index/sai/plan/QueryController.java +++ b/src/java/org/apache/cassandra/index/sai/plan/QueryController.java @@ -148,22 +148,15 @@ public class QueryController if (key == null) throw new IllegalArgumentException("non-null key required"); - try - { - SinglePartitionReadCommand partition = SinglePartitionReadCommand.create(cfs.metadata(), - command.nowInSec(), - command.columnFilter(), - RowFilter.none(), - DataLimits.NONE, - key.partitionKey(), - makeFilter(key)); + SinglePartitionReadCommand partition = SinglePartitionReadCommand.create(cfs.metadata(), + command.nowInSec(), + command.columnFilter(), + RowFilter.none(), + DataLimits.NONE, + key.partitionKey(), + makeFilter(key)); - return partition.queryMemtableAndDisk(cfs, executionController); - } - finally - { - queryContext.checkpoint(); - } + return partition.queryMemtableAndDisk(cfs, executionController); } /** diff --git a/src/java/org/apache/cassandra/index/sai/plan/StorageAttachedIndexSearcher.java b/src/java/org/apache/cassandra/index/sai/plan/StorageAttachedIndexSearcher.java index abfd6c9e3f..c5976de48f 100644 --- a/src/java/org/apache/cassandra/index/sai/plan/StorageAttachedIndexSearcher.java +++ b/src/java/org/apache/cassandra/index/sai/plan/StorageAttachedIndexSearcher.java @@ -153,11 +153,6 @@ public class StorageAttachedIndexSearcher implements Index.Searcher @Override public UnfilteredRowIterator computeNext() { - // IMPORTANT: The correctness of the entire query pipeline relies on the fact that we consume a token - // and materialize its keys before moving on to the next token in the flow. This sequence must not be broken - // with toList() or similar. (Both the union and intersection flow constructs, to avoid excessive object - // allocation, reuse their token mergers as they process individual positions on the ring.) - if (resultKeyIterator == null) return endOfData(); @@ -379,6 +374,7 @@ public class StorageAttachedIndexSearcher implements Index.Searcher try (UnfilteredRowIterator partition = queryController.queryStorage(key, executionController)) { queryContext.partitionsRead++; + queryContext.checkpoint(); return applyIndexFilter(key, partition, filterTree, queryContext); } diff --git a/src/java/org/apache/cassandra/index/sai/postings/RangePostingList.java b/src/java/org/apache/cassandra/index/sai/postings/RangePostingList.java deleted file mode 100644 index 9de2f1f4f6..0000000000 --- a/src/java/org/apache/cassandra/index/sai/postings/RangePostingList.java +++ /dev/null @@ -1,88 +0,0 @@ -/* - * 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.index.sai.postings; - -import java.io.IOException; - -import org.apache.cassandra.index.sai.QueryContext; - -public class RangePostingList implements PostingList -{ - private final PostingList wrapped; - private final long rowIdOffset; - private final long minimum; - private final long maximum; - private final long count; - private final QueryContext queryContext; - - public RangePostingList(PostingList wrapped, long rowIdOffset, long minimum, long maximum, long count, QueryContext queryContext) - { - this.wrapped = wrapped; - this.rowIdOffset = rowIdOffset; - this.minimum = minimum; - this.maximum = maximum; - this.count = count; - this.queryContext = queryContext; - } - - @Override - public long minimum() - { - return minimum; - } - - @Override - public long maximum() - { - return maximum; - } - - @Override - public long nextPosting() throws IOException - { - queryContext.checkpoint(); - return nextSSTableRowId(wrapped.nextPosting()); - } - - @Override - public long size() - { - return count; - } - - @Override - public long advance(long targetRowID) throws IOException - { - queryContext.checkpoint(); - long segmentRowId = targetRowID - rowIdOffset; - assert segmentRowId >= 0; - return nextSSTableRowId(wrapped.advance(segmentRowId)); - } - - @Override - public void close() - { - wrapped.close(); - } - - private long nextSSTableRowId(long segmentRowId) - { - return segmentRowId == PostingList.END_OF_STREAM ? PostingList.END_OF_STREAM : segmentRowId + rowIdOffset; - } -}