From 7873cf845e1d513026b6836769b7cbd4d237c2aa Mon Sep 17 00:00:00 2001 From: Adrien Grand Date: Wed, 9 May 2018 15:12:23 +0200 Subject: [PATCH] LUCENE-8296: PendingDeletes may no longer write to live docs after they are shared. --- .../apache/lucene/index/PendingDeletes.java | 51 ++++++++----------- .../lucene/index/PendingSoftDeletes.java | 8 +-- .../lucene/index/ReadersAndUpdates.java | 11 ++-- .../lucene/index/TestPendingDeletes.java | 10 ++-- .../lucene/index/TestPendingSoftDeletes.java | 2 - 5 files changed, 29 insertions(+), 53 deletions(-) diff --git a/lucene/core/src/java/org/apache/lucene/index/PendingDeletes.java b/lucene/core/src/java/org/apache/lucene/index/PendingDeletes.java index 354f9d18c3b..4bd90cbccc4 100644 --- a/lucene/core/src/java/org/apache/lucene/index/PendingDeletes.java +++ b/lucene/core/src/java/org/apache/lucene/index/PendingDeletes.java @@ -33,15 +33,11 @@ import org.apache.lucene.util.IOUtils; */ class PendingDeletes { protected final SegmentCommitInfo info; - // True if the current liveDocs is referenced by an - // external NRT reader: - protected boolean liveDocsShared; - // Holds the current shared (readable and writable) - // liveDocs. This is null when there are no deleted - // docs, and it's copy-on-write (cloned whenever we need - // to change it but it's been shared to an external NRT - // reader). + // Read-only live docs, null until live docs are initialized or if all docs are alive private Bits liveDocs; + // Writeable live docs, null if this instance is not ready to accept writes, in which + // case getMutableBits needs to be called + private FixedBitSet writeableLiveDocs; protected int pendingDeleteCount; private boolean liveDocsInitialized; @@ -59,7 +55,6 @@ class PendingDeletes { private PendingDeletes(SegmentCommitInfo info, Bits liveDocs, boolean liveDocsInitialized) { this.info = info; - liveDocsShared = true; this.liveDocs = liveDocs; pendingDeleteCount = 0; this.liveDocsInitialized = liveDocsInitialized; @@ -70,24 +65,23 @@ class PendingDeletes { // if we pull mutable bits but we haven't been initialized something is completely off. // this means we receive deletes without having the bitset that is on-disk ready to be cloned assert liveDocsInitialized : "can't delete if liveDocs are not initialized"; - if (liveDocsShared) { + if (writeableLiveDocs == null) { // Copy on write: this means we've cloned a // SegmentReader sharing the current liveDocs // instance; must now make a private clone so we can // change it: - FixedBitSet mutableBits = new FixedBitSet(info.info.maxDoc()); - mutableBits.set(0, info.info.maxDoc()); + writeableLiveDocs = new FixedBitSet(info.info.maxDoc()); + writeableLiveDocs.set(0, info.info.maxDoc()); if (liveDocs != null) { for (int i = 0; i < liveDocs.length(); ++i) { if (liveDocs.get(i) == false) { - mutableBits.clear(i); + writeableLiveDocs.clear(i); } } } - liveDocs = mutableBits; - liveDocsShared = false; + liveDocs = writeableLiveDocs; } - return (FixedBitSet) liveDocs; + return writeableLiveDocs; } @@ -100,7 +94,6 @@ class PendingDeletes { FixedBitSet mutableBits = getMutableBits(); assert mutableBits != null; assert docID >= 0 && docID < mutableBits.length() : "out of bounds: docid=" + docID + " liveDocsLength=" + mutableBits.length() + " seg=" + info.info.name + " maxDoc=" + info.info.maxDoc(); - assert !liveDocsShared; final boolean didDelete = mutableBits.get(docID); if (didDelete) { mutableBits.clear(docID); @@ -110,20 +103,19 @@ class PendingDeletes { } /** - * Should be called if the live docs returned from {@link #getLiveDocs()} are shared outside of the - * {@link ReadersAndUpdates} + * Returns a snapshot of the current live docs. */ - void liveDocsShared() { - liveDocsShared = true; + Bits getLiveDocs() { + // Prevent modifications to the returned live docs + writeableLiveDocs = null; + return liveDocs; } /** - * Returns the current live docs or null if all docs are live. The returned instance might be mutable or is mutated behind the scenes. - * If the returned live docs are shared outside of the ReadersAndUpdates {@link #liveDocsShared()} should be called - * first. + * Returns a snapshot of the hard live docs. */ - Bits getLiveDocs() { - return liveDocs; + Bits getHardLiveDocs() { + return getLiveDocs(); } /** @@ -138,6 +130,7 @@ class PendingDeletes { */ void onNewReader(CodecReader reader, SegmentCommitInfo info) throws IOException { if (liveDocsInitialized == false) { + assert writeableLiveDocs == null; if (reader.hasDeletions()) { // we only initialize this once either in the ctor or here // if we use the live docs from a reader it has to be in a situation where we don't @@ -145,7 +138,6 @@ class PendingDeletes { assert pendingDeleteCount == 0 : "pendingDeleteCount: " + pendingDeleteCount; liveDocs = reader.getLiveDocs(); assert liveDocs == null || assertCheckLiveDocs(liveDocs, info.info.maxDoc(), info.getDelCount()); - liveDocsShared = true; } liveDocsInitialized = true; } @@ -175,7 +167,7 @@ class PendingDeletes { StringBuilder sb = new StringBuilder(); sb.append("PendingDeletes(seg=").append(info); sb.append(" numPendingDeletes=").append(pendingDeleteCount); - sb.append(" liveDocsShared=").append(liveDocsShared); + sb.append(" writeable=").append(writeableLiveDocs != null); return sb.toString(); } @@ -246,7 +238,4 @@ class PendingDeletes { return policy.numDeletesToMerge(info, numPendingDeletes(), readerIOSupplier); } - Bits getHardLiveDocs() { - return liveDocs; - } } diff --git a/lucene/core/src/java/org/apache/lucene/index/PendingSoftDeletes.java b/lucene/core/src/java/org/apache/lucene/index/PendingSoftDeletes.java index 3809304e893..637a90cf070 100644 --- a/lucene/core/src/java/org/apache/lucene/index/PendingSoftDeletes.java +++ b/lucene/core/src/java/org/apache/lucene/index/PendingSoftDeletes.java @@ -195,13 +195,9 @@ final class PendingSoftDeletes extends PendingDeletes { } } + @Override Bits getHardLiveDocs() { - return hardDeletes.getHardLiveDocs(); + return hardDeletes.getLiveDocs(); } - @Override - void liveDocsShared() { - super.liveDocsShared(); - hardDeletes.liveDocsShared(); - } } diff --git a/lucene/core/src/java/org/apache/lucene/index/ReadersAndUpdates.java b/lucene/core/src/java/org/apache/lucene/index/ReadersAndUpdates.java index a02d26ac024..1a96c1347a9 100644 --- a/lucene/core/src/java/org/apache/lucene/index/ReadersAndUpdates.java +++ b/lucene/core/src/java/org/apache/lucene/index/ReadersAndUpdates.java @@ -234,7 +234,6 @@ final class ReadersAndUpdates { } // force new liveDocs Bits liveDocs = pendingDeletes.getLiveDocs(); - markAsShared(); if (liveDocs != null) { return new SegmentReader(reader.getSegmentInfo(), reader, liveDocs, info.info.maxDoc() - info.getDelCount() - pendingDeletes.numPendingDeletes()); @@ -263,6 +262,9 @@ final class ReadersAndUpdates { return reader; } + /** + * Returns a snapshot of the live docs. + */ public synchronized Bits getLiveDocs() { return pendingDeletes.getLiveDocs(); } @@ -720,8 +722,6 @@ final class ReadersAndUpdates { assert pendingDeletes.getLiveDocs() != null; reader = createNewReaderWithLatestLiveDocs(reader); } - - markAsShared(); assert verifyDocCounts(); return new MergeReader(reader, pendingDeletes.getHardLiveDocs()); @@ -754,11 +754,6 @@ final class ReadersAndUpdates { return pendingDeletes.isFullyDeleted(this::getLatestReader); } - private final void markAsShared() { - assert Thread.holdsLock(this); - pendingDeletes.liveDocsShared(); // this is not costly we can just call it even if it's already marked as shared - } - boolean keepFullyDeletedSegment(MergePolicy mergePolicy) throws IOException { return mergePolicy.keepFullyDeletedSegment(this::getLatestReader); } diff --git a/lucene/core/src/test/org/apache/lucene/index/TestPendingDeletes.java b/lucene/core/src/test/org/apache/lucene/index/TestPendingDeletes.java index 7c6891e8921..ecc2d4de51e 100644 --- a/lucene/core/src/test/org/apache/lucene/index/TestPendingDeletes.java +++ b/lucene/core/src/test/org/apache/lucene/index/TestPendingDeletes.java @@ -52,18 +52,16 @@ public class TestPendingDeletes extends LuceneTestCase { assertFalse(liveDocs.get(docToDelete)); assertFalse(deletes.delete(docToDelete)); // delete again - // make sure we are live ie. mutable assertTrue(liveDocs.get(8)); assertTrue(deletes.delete(8)); - assertFalse(liveDocs.get(8)); + assertTrue(liveDocs.get(8)); // we have a snapshot assertEquals(2, deletes.numPendingDeletes()); - deletes.liveDocsShared(); - - // make sure we are live ie. mutable assertTrue(liveDocs.get(9)); assertTrue(deletes.delete(9)); assertTrue(liveDocs.get(9)); + + // now make sure new live docs see the deletions liveDocs = deletes.getLiveDocs(); assertFalse(liveDocs.get(9)); assertFalse(liveDocs.get(8)); @@ -83,7 +81,7 @@ public class TestPendingDeletes extends LuceneTestCase { boolean secondDocDeletes = random().nextBoolean(); deletes.delete(5); if (secondDocDeletes) { - deletes.liveDocsShared(); + deletes.getLiveDocs(); deletes.delete(2); } assertEquals(-1, commitInfo.getDelGen()); diff --git a/lucene/core/src/test/org/apache/lucene/index/TestPendingSoftDeletes.java b/lucene/core/src/test/org/apache/lucene/index/TestPendingSoftDeletes.java index aeb5819c72f..903f84726f6 100644 --- a/lucene/core/src/test/org/apache/lucene/index/TestPendingSoftDeletes.java +++ b/lucene/core/src/test/org/apache/lucene/index/TestPendingSoftDeletes.java @@ -73,7 +73,6 @@ public class TestPendingSoftDeletes extends TestPendingDeletes { assertNull(pendingSoftDeletes.getHardLiveDocs()); // pass reader again Bits liveDocs = pendingSoftDeletes.getLiveDocs(); - pendingSoftDeletes.liveDocsShared(); pendingSoftDeletes.onNewReader(segmentReader, segmentInfo); assertEquals(1, pendingSoftDeletes.numPendingDeletes()); assertSame(liveDocs, pendingSoftDeletes.getLiveDocs()); @@ -189,7 +188,6 @@ public class TestPendingSoftDeletes extends TestPendingDeletes { assertTrue(deletes.getLiveDocs().get(0)); assertFalse(deletes.getLiveDocs().get(1)); assertTrue(deletes.getLiveDocs().get(2)); - deletes.liveDocsShared(); Bits liveDocs = deletes.getLiveDocs(); deletes.onNewReader(segmentReader, segmentInfo); // no changes we don't apply updates twice