You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@ignite.apache.org by sd...@apache.org on 2023/03/01 16:29:51 UTC
[ignite-3] 01/01: IGNITE-18882 Don't store tombstones if there is no other version of row
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 <sa...@yandex.ru>
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;