This is an automated email from the ASF dual-hosted git repository. sdanilov pushed a commit to branch ignite-18882 in repository https://gitbox.apache.org/repos/asf/ignite-3.git
commit 11248f8031bdd4fbe815ce95e7dc7a95c5d214b1 Author: Semyon Danilov <[email protected]> AuthorDate: Wed Mar 1 20:29:42 2023 +0400 IGNITE-18882 Don't store tombstones if there is no other version of row --- .../storage/AbstractMvPartitionStorageGcTest.java | 15 ++++++-- .../storage/impl/TestMvPartitionStorage.java | 4 +++ .../mv/AddWriteCommittedInvokeClosure.java | 6 ++++ .../pagememory/mv/CommitWriteInvokeClosure.java | 7 ++++ .../internal/storage/rocksdb/GarbageCollector.java | 40 +++++++++++++--------- .../storage/rocksdb/RocksDbMvPartitionStorage.java | 1 + 6 files changed, 54 insertions(+), 19 deletions(-) diff --git a/modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/AbstractMvPartitionStorageGcTest.java b/modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/AbstractMvPartitionStorageGcTest.java index f98e1822f3..9911115db5 100644 --- a/modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/AbstractMvPartitionStorageGcTest.java +++ b/modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/AbstractMvPartitionStorageGcTest.java @@ -21,7 +21,6 @@ import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; import org.apache.ignite.internal.hlc.HybridTimestamp; -import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; /** @@ -132,7 +131,6 @@ public abstract class AbstractMvPartitionStorageGcTest extends BaseMvPartitionSt } @Test - @Disabled("https://issues.apache.org/jira/browse/IGNITE-18882";) void testVacuumsSecondRowIfTombstoneIsFirst() { addAndCommit(null); @@ -144,4 +142,17 @@ public abstract class AbstractMvPartitionStorageGcTest extends BaseMvPartitionSt assertRowMatches(row.binaryRow(), TABLE_ROW); } + + @Test + void testVacuumsSecondRowIfTombstoneIsFirst2() { + addWriteCommitted(ROW_ID, null, clock.now()); + + addWriteCommitted(ROW_ID, TABLE_ROW, clock.now()); + + addWriteCommitted(ROW_ID, TABLE_ROW2, clock.now()); + + BinaryRowAndRowId row = pollForVacuum(HybridTimestamp.MAX_VALUE); + + assertRowMatches(row.binaryRow(), TABLE_ROW); + } } diff --git a/modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/impl/TestMvPartitionStorage.java b/modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/impl/TestMvPartitionStorage.java index 6afd07d7dd..67f926b54e 100644 --- a/modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/impl/TestMvPartitionStorage.java +++ b/modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/impl/TestMvPartitionStorage.java @@ -271,6 +271,10 @@ public class TestMvPartitionStorage implements MvPartitionStorage { // Calling it from the compute is fine. Concurrent writes of the same row are impossible, and if we call the compute closure // several times, the same tuple will be inserted into the GC queue (timestamp and rowId don't change in this case). gcQueue.add(committedVersionChain); + } else { + if (committedVersionChain.row == null) { + return null; + } } return committedVersionChain; diff --git a/modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/AddWriteCommittedInvokeClosure.java b/modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/AddWriteCommittedInvokeClosure.java index 3fddbc1249..182699cceb 100644 --- a/modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/AddWriteCommittedInvokeClosure.java +++ b/modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/AddWriteCommittedInvokeClosure.java @@ -81,6 +81,12 @@ class AddWriteCommittedInvokeClosure implements InvokeClosure<VersionChain> { throw new StorageException("Write intent exists: [rowId={}, {}]", oldRow.rowId(), storage.createStorageInfo()); } + if (row == null && oldRow == null) { + operationType = OperationType.NOOP; + + return; + } + if (oldRow == null) { operationType = OperationType.PUT; diff --git a/modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/CommitWriteInvokeClosure.java b/modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/CommitWriteInvokeClosure.java index 3a9b6e28e5..34ad755db5 100644 --- a/modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/CommitWriteInvokeClosure.java +++ b/modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/CommitWriteInvokeClosure.java @@ -80,6 +80,13 @@ class CommitWriteInvokeClosure implements InvokeClosure<VersionChain> { RowVersion current = storage.readRowVersion(oldRow.headLink(), DONT_LOAD_VALUE); RowVersion next = oldRow.hasNextLink() ? storage.readRowVersion(oldRow.nextLink(), DONT_LOAD_VALUE) : null; + if (next == null && current.isTombstone()) { + // Previous version doesn't exist and current version is a tombstone, delete current version. + operationType = OperationType.REMOVE; + + return; + } + // If the previous and current version are tombstones, then delete the current version. if (next != null && current.isTombstone() && next.isTombstone()) { toRemove = current; diff --git a/modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/GarbageCollector.java b/modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/GarbageCollector.java index d150deac98..01743cd39d 100644 --- a/modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/GarbageCollector.java +++ b/modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/GarbageCollector.java @@ -105,8 +105,6 @@ class GarbageCollector { throws RocksDBException { ColumnFamilyHandle partCf = helper.partCf; - boolean newAndPrevTombstones = false; - // Try find previous value for the row id. ByteBuffer keyBuffer = MV_KEY_BUFFER.get(); keyBuffer.clear(); @@ -117,7 +115,7 @@ class GarbageCollector { it.seek(keyBuffer); if (invalid(it)) { - return false; + return isNewValueTombstone; } keyBuffer.clear(); @@ -126,28 +124,36 @@ class GarbageCollector { RowId readRowId = helper.getRowId(keyBuffer, ROW_ID_OFFSET); - if (readRowId.equals(rowId)) { - // Found previous value. - assert keyLen == MAX_KEY_SIZE; // Can not be write-intent. + if (!readRowId.equals(rowId)) { + return isNewValueTombstone; + } - if (isNewValueTombstone) { - // If new value is a tombstone, lets check if previous value was also a tombstone. - int valueSize = it.value(EMPTY_DIRECT_BUFFER); + // Found previous value. + assert keyLen == MAX_KEY_SIZE; // Can not be write-intent. - newAndPrevTombstones = valueSize == 0; + if (isNewValueTombstone) { + // If new value is a tombstone, lets check if previous value was also a tombstone. + int valueSize = it.value(EMPTY_DIRECT_BUFFER); + + if (valueSize == 0) { + return true; } + } - if (!newAndPrevTombstones) { - keyBuffer.clear(); + keyBuffer.clear(); - helper.putGcKey(keyBuffer, rowId, timestamp); + helper.putGcKey(keyBuffer, rowId, timestamp); - writeBatch.put(gcQueueCf, keyBuffer, EMPTY_DIRECT_BUFFER); - } - } + writeBatch.put(gcQueueCf, keyBuffer, EMPTY_DIRECT_BUFFER); } - return newAndPrevTombstones; + return false; + } + + enum Res { + NO_PREVIOUS_VALUE, + PREVIOUS_VALUE_TOMBSTONE, + HAS_PREVIOUS_VALUE } /** diff --git a/modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/RocksDbMvPartitionStorage.java b/modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/RocksDbMvPartitionStorage.java index 31278c1a90..5deab406bf 100644 --- a/modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/RocksDbMvPartitionStorage.java +++ b/modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/RocksDbMvPartitionStorage.java @@ -67,6 +67,7 @@ import org.apache.ignite.internal.storage.RowId; import org.apache.ignite.internal.storage.StorageException; import org.apache.ignite.internal.storage.StorageRebalanceException; import org.apache.ignite.internal.storage.TxIdMismatchException; +import org.apache.ignite.internal.storage.rocksdb.GarbageCollector.Res; import org.apache.ignite.internal.storage.util.StorageState; import org.apache.ignite.internal.util.ArrayUtils; import org.apache.ignite.internal.util.Cursor;
