IGNITE-17859 Update indexes on data modifications #1191

korlov42 · 2022-10-12T06:22:52Z

No description provided.

modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java

...table/src/main/java/org/apache/ignite/internal/table/distributed/raft/PartitionListener.java

...n/java/org/apache/ignite/internal/table/distributed/replicator/PartitionReplicaListener.java

sanpwc · 2022-10-12T18:33:26Z

...n/java/org/apache/ignite/internal/table/distributed/replicator/PartitionReplicaListener.java

+
+            assert indexRow != null;
+
+            locks[idx++] = lockManager.acquire(txId, new LockKey(indexId, indexRow), LockMode.X);


Not that simple.Different type of indexes will require different type of locks on different operations on different entries

different operations
Insertion in sorted non-unique index expect following locks to be taken:

IX_short(nextKey)

X_commit(currentKey) if nextKey previously locked in S, X or SIX mode

IX_commit(currentKey) otherwise

Select
S_commit(currentKey)

different entries
More precisely select in non-unique is not always about locking current key, but sometimes is about locking +INF

If the currentKey is bigger than the upper bound, return NOT_FOUND but take a lock anyway.

If currentKey matches the upper bound no need to take a lock on the next key.

If the upper bound is null, take a lock on +INF.

If nothing is found, take a lock on +INF.

Different type of indexes
That part is pretty straight forward, hash index on insert requires different set of locks to be taken in comparison to sorted index, same is about unique/non-unique indexes.

In order to properly hide given difficulties, I'd favor using Alexey's proposal with wrapping IndexStorages with decorators that'll add index specific lock stuff to put, get and remove methods. That's why, I would expect
Supplier<List<IndexStorage>> activeIndexes to return list of such decorator instead of IndexStorage's.

...n/java/org/apache/ignite/internal/table/distributed/replicator/PartitionReplicaListener.java

...table/src/main/java/org/apache/ignite/internal/table/distributed/raft/PartitionListener.java

…teBuffer to index key

...es/table/src/integrationTest/java/org/apache/ignite/distributed/ItInternalTableScanTest.java

...les/table/src/integrationTest/java/org/apache/ignite/distributed/ItTablePersistenceTest.java

...ge-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/RocksDbTableStorage.java

modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java

- rename local variable in RocksDbTableStorage#getOrCreateHashIndex - move schema descriptor to a constant in ItInternalTableScanTest - add missing param description to javadoc - remove unused code - fix failed tests

ibessonov · 2022-10-24T08:44:34Z

modules/index/src/main/java/org/apache/ignite/internal/index/IndexManager.java

@@ -380,6 +403,50 @@ private SortedIndexDescriptor convert(SortedIndexView indexView) {
        );
    }

+    private CompletableFuture<BinaryTuple> toIndexKey(TableIndexView indexView, BinaryRow tableRow) {


This one is very inefficient. We're planning to redo it in the future, right?

Decided to rework this a bit. Now converter should be created once for each schema

modules/storage-api/src/main/java/org/apache/ignite/internal/storage/engine/MvTableStorage.java

...ge-api/src/testFixtures/java/org/apache/ignite/internal/storage/impl/TestMvTableStorage.java

...main/java/org/apache/ignite/internal/storage/rocksdb/index/RocksDbBinaryTupleComparator.java

modules/table/src/main/java/org/apache/ignite/internal/table/distributed/PkStorage.java

modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java

ibessonov · 2022-10-24T08:56:55Z

modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java

+     * @param indexId An index id os the index to register.
+     * @param searchRowResolver Function which converts given table row to an index key.
+     */
+    public void registerHashIndex(UUID tableId, UUID indexId, Function<BinaryRow, CompletableFuture<BinaryTuple>> searchRowResolver) {


Why does it have to be so complicated? Is this because of our weird modules dependencies and the fact that indexes are in their own module?

ibessonov · 2022-10-24T08:59:59Z

modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java

+                        BinaryTuplePrefix prefix = BinaryTuplePrefix.fromBinaryTuple(key);
+
+                        // find next key
+                        Cursor<IndexRow> cursor = storage.scan(prefix, null, SortedIndexStorage.GREATER);


Note for the future: we should add a hint that only one entry will be read. This will allow us to make some optimizations in the storage

ibessonov · 2022-10-24T09:09:02Z

...n/java/org/apache/ignite/internal/table/distributed/replicator/PartitionReplicaListener.java

+     * @param <T> A type of the value returned by action.
+     * @return A future object representing the result of the given action.
+     */
+    private <T> CompletableFuture<T> resolveRowByPk(


This method has the same name and comment as the previous one. But it does some locks magic. Should we rename it and update the documentation?

Well, I assume (mistakenly) this should be obvious from functions signature: resolve by key and timestamp => RO => just read the resolve the row; resolve by key and txId => RW => acquire all necessary lock before resolving

modules/index/src/main/java/org/apache/ignite/internal/index/IndexManager.java

...les/table/src/integrationTest/java/org/apache/ignite/distributed/ItTablePersistenceTest.java

modules/table/src/main/java/org/apache/ignite/internal/table/distributed/IndexLocker.java

modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java

...table/src/main/java/org/apache/ignite/internal/table/distributed/raft/PartitionListener.java

...n/java/org/apache/ignite/internal/table/distributed/replicator/PartitionReplicaListener.java

- rename variable in test storage implementation - remove artifacts of merge conflict - fix artifacts of copy-paste in IndexLocker interface

# Conflicts: # modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/PartitionReplicaListener.java # modules/table/src/main/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImpl.java # modules/table/src/test/java/org/apache/ignite/internal/table/distributed/raft/PartitionCommandListenerTest.java # modules/table/src/test/java/org/apache/ignite/internal/table/impl/DummyInternalTableImpl.java

# Conflicts: # modules/table/src/integrationTest/java/org/apache/ignite/distributed/ItAbstractInternalTableScanTest.java # modules/table/src/integrationTest/java/org/apache/ignite/distributed/ItTablePersistenceTest.java # modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java # modules/table/src/main/java/org/apache/ignite/internal/table/distributed/raft/PartitionListener.java # modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/PartitionReplicaListener.java # modules/table/src/test/java/org/apache/ignite/internal/table/distributed/replication/PartitionReplicaListenerTest.java

sanpwc · 2022-10-26T09:38:36Z

modules/core/src/main/java/org/apache/ignite/internal/util/Lazy.java

+ * @param <T> Type of the value.
+ */
+public class Lazy<T> {
+    private static final Supplier<?> EMPTY = () -> {


Why do we need empty supplier?

To replace the one provided in construct. The supplier may prevent the object be collected by garbage collector

sanpwc · 2022-10-26T09:40:22Z

modules/core/src/main/java/org/apache/ignite/internal/util/Lazy.java

+        throw new IllegalStateException("Should not be called");
+    };
+
+    private volatile Supplier<T> supplier;


Seems that final should be enough.

not enough, if want to help GC to collect captured objects

modules/core/src/main/java/org/apache/ignite/internal/util/Lazy.java

sanpwc reviewed Oct 12, 2022

View reviewed changes

WIP: add row to indexes on insert

f97ff47

korlov42 force-pushed the ignite-17859 branch from eaf7a8c to f97ff47 Compare October 18, 2022 09:03

korlov42 added 6 commits October 18, 2022 16:13

minors

2f863e5

minors #2

6cbce42

fix failed tests

27c112f

fix failed tests #2

4fff530

merge "main" branch

4bda1dc

fix failed tests #3

0308be9

korlov42 force-pushed the ignite-17859 branch from c371553 to 0308be9 Compare October 19, 2022 18:03

introduce PkStorage facade to encapsulate logic of conversion from By…

3dce8db

…teBuffer to index key

AMashenkov reviewed Oct 20, 2022

View reviewed changes

...es/table/src/integrationTest/java/org/apache/ignite/distributed/ItInternalTableScanTest.java Outdated Show resolved Hide resolved

AMashenkov reviewed Oct 20, 2022

View reviewed changes

...les/table/src/integrationTest/java/org/apache/ignite/distributed/ItTablePersistenceTest.java Show resolved Hide resolved

AMashenkov reviewed Oct 20, 2022

View reviewed changes

...ge-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/RocksDbTableStorage.java Outdated Show resolved Hide resolved

AMashenkov reviewed Oct 20, 2022

View reviewed changes

modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java Outdated Show resolved Hide resolved

korlov42 added 3 commits October 20, 2022 12:50

fixes after review

a2fc999

- rename local variable in RocksDbTableStorage#getOrCreateHashIndex - move schema descriptor to a constant in ItInternalTableScanTest - add missing param description to javadoc - remove unused code - fix failed tests

split TableIndex onto TableLocker and StorageAdapter

2b592b7

rework row resolving in a way to accepts action closure

6d9c750

korlov42 marked this pull request as ready for review October 24, 2022 07:52

ibessonov reviewed Oct 24, 2022

View reviewed changes