Skip to content

Commit 0d18b95

Browse files
brfrn169brfrn169
committed
Prevent overwriting read set in Consensus Commit
1 parent 7f2aa1e commit 0d18b95

File tree

3 files changed

+131
-9
lines changed

3 files changed

+131
-9
lines changed

‎core/src/main/java/com/scalar/db/transaction/consensuscommit/CrudHandler.java‎

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -173,11 +173,10 @@ Optional<TransactionResult> read(@Nullable Snapshot.Key key, Get get) throws Cru
173173
}
174174

175175
if (result.isPresent() || get.getConjunctions().isEmpty()) {
176-
// Keep the read set latest to create before image by using the latest record (result)
177-
// because another conflicting transaction might have updated the record after this
178-
// transaction read it first. However, we update it only if a get operation has no
179-
// conjunction or the result exists. This is because we don’t know whether the record
180-
// actually exists or not due to the conjunction.
176+
// We put the result into the read set only if a get operation has no conjunction or the
177+
// result exists. This is because we don’t know whether the record actually exists or not
178+
// due to the conjunction.
179+
181180
if (key != null) {
182181
putIntoReadSetInSnapshot(key, result);
183182
} else {
@@ -285,9 +284,6 @@ private Optional<TransactionResult> processScanResult(
285284
}
286285

287286
if (ret.isPresent()) {
288-
// We always update the read set to create before image by using the latest record (result)
289-
// because another conflicting transaction might have updated the record after this
290-
// transaction read it first.
291287
putIntoReadSetInSnapshot(key, ret);
292288
}
293289

@@ -327,7 +323,7 @@ public void closeScanners() throws CrudException {
327323

328324
private void putIntoReadSetInSnapshot(Snapshot.Key key, Optional<TransactionResult> result) {
329325
// In read-only mode, we don't need to put the result into the read set
330-
if (!readOnly) {
326+
if (!readOnly && !snapshot.containsKeyInReadSet(key)) {
331327
snapshot.putIntoReadSet(key, result);
332328
}
333329
}

‎core/src/test/java/com/scalar/db/transaction/consensuscommit/CrudHandlerTest.java‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -539,6 +539,45 @@ public void get_ForNonExistingTable_ShouldThrowIllegalArgumentException()
539539
assertThatThrownBy(() -> handler.get(get)).isInstanceOf(IllegalArgumentException.class);
540540
}
541541

542+
@Test
543+
public void get_DifferentGetButSameRecordReturned_ShouldNotOverwriteReadSet()
544+
throws ExecutionException, CrudException {
545+
// Arrange
546+
Get get1 = prepareGet();
547+
Get get2 = Get.newBuilder(get1).where(column(ANY_NAME_3).isEqualToText(ANY_TEXT_3)).build();
548+
Get getForStorage1 = toGetForStorageFrom(get1);
549+
Get getForStorage2 =
550+
Get.newBuilder(get2)
551+
.clearProjections()
552+
.projections(TRANSACTION_TABLE_METADATA.getAfterImageColumnNames())
553+
.clearConditions()
554+
.where(column(ANY_NAME_3).isEqualToText(ANY_TEXT_3))
555+
.or(column(Attribute.BEFORE_PREFIX + ANY_NAME_3).isEqualToText(ANY_TEXT_3))
556+
.consistency(Consistency.LINEARIZABLE)
557+
.build();
558+
Result result = prepareResult(TransactionState.COMMITTED);
559+
Optional<TransactionResult> expected = Optional.of(new TransactionResult(result));
560+
Snapshot.Key key = new Snapshot.Key(getForStorage1);
561+
when(snapshot.getResult(any(), any())).thenReturn(expected).thenReturn(expected);
562+
when(snapshot.containsKeyInReadSet(key)).thenReturn(false).thenReturn(true);
563+
when(storage.get(any())).thenReturn(Optional.of(result));
564+
565+
// Act
566+
Optional<Result> results1 = handler.get(get1);
567+
Optional<Result> results2 = handler.get(get2);
568+
569+
// Assert
570+
assertThat(results1)
571+
.isEqualTo(
572+
Optional.of(
573+
new FilteredResult(
574+
expected.get(), Collections.emptyList(), TABLE_METADATA, false)));
575+
assertThat(results2).isEqualTo(results1);
576+
verify(storage).get(getForStorage1);
577+
verify(storage).get(getForStorage2);
578+
verify(snapshot).putIntoReadSet(key, expected);
579+
}
580+
542581
@ParameterizedTest
543582
@EnumSource(ScanType.class)
544583
void scanOrGetScanner_ResultGivenFromStorage_ShouldUpdateSnapshotAndReturn(ScanType scanType)

‎integration-test/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitSpecificIntegrationTestBase.java‎

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package com.scalar.db.transaction.consensuscommit;
22

33
import static com.scalar.db.api.ConditionBuilder.column;
4+
import static com.scalar.db.api.ConditionBuilder.updateIf;
45
import static org.assertj.core.api.Assertions.assertThat;
56
import static org.assertj.core.api.Assertions.assertThatCode;
67
import static org.assertj.core.api.Assertions.assertThatThrownBy;
@@ -45,6 +46,7 @@
4546
import com.scalar.db.exception.transaction.CrudConflictException;
4647
import com.scalar.db.exception.transaction.CrudException;
4748
import com.scalar.db.exception.transaction.PreparationConflictException;
49+
import com.scalar.db.exception.transaction.RollbackException;
4850
import com.scalar.db.exception.transaction.TransactionException;
4951
import com.scalar.db.exception.transaction.UnknownTransactionStatusException;
5052
import com.scalar.db.io.DataType;
@@ -5927,6 +5929,91 @@ public void getScanner_InReadOnlyMode_WithSerializable_ShouldNotThrowAnyExceptio
59275929
assertThat(results.get(1).getInt(BALANCE)).isEqualTo(INITIAL_BALANCE);
59285930
}
59295931

5932+
@Test
5933+
public void
5934+
commit_ConflictingExternalUpdate_DifferentGetButSameRecordReturned_ShouldThrowCommitConflictExceptionAndPreserveExternalChanges()
5935+
throws UnknownTransactionStatusException, CrudException, RollbackException {
5936+
// Arrange
5937+
manager.insert(
5938+
Insert.newBuilder()
5939+
.namespace(namespace1)
5940+
.table(TABLE_1)
5941+
.partitionKey(Key.ofInt(ACCOUNT_ID, 0))
5942+
.clusteringKey(Key.ofInt(ACCOUNT_TYPE, 0))
5943+
.intValue(BALANCE, INITIAL_BALANCE)
5944+
.build());
5945+
5946+
// Act Assert
5947+
DistributedTransaction transaction = manager.begin();
5948+
5949+
// Retrieve the record
5950+
Optional<Result> result =
5951+
transaction.get(
5952+
Get.newBuilder()
5953+
.namespace(namespace1)
5954+
.table(TABLE_1)
5955+
.partitionKey(Key.ofInt(ACCOUNT_ID, 0))
5956+
.clusteringKey(Key.ofInt(ACCOUNT_TYPE, 0))
5957+
.build());
5958+
5959+
assertThat(result).isPresent();
5960+
assertThat(result.get().getInt(ACCOUNT_ID)).isEqualTo(0);
5961+
assertThat(result.get().getInt(ACCOUNT_TYPE)).isEqualTo(0);
5962+
assertThat(result.get().getInt(BALANCE)).isEqualTo(INITIAL_BALANCE);
5963+
5964+
// Update the balance of the record
5965+
transaction.update(
5966+
Update.newBuilder()
5967+
.namespace(namespace1)
5968+
.table(TABLE_1)
5969+
.partitionKey(Key.ofInt(ACCOUNT_ID, 0))
5970+
.clusteringKey(Key.ofInt(ACCOUNT_TYPE, 0))
5971+
.condition(updateIf(column(BALANCE).isEqualToInt(INITIAL_BALANCE)).build())
5972+
.intValue(BALANCE, 100)
5973+
.build());
5974+
5975+
// Update the balance of the record by another transaction
5976+
manager.update(
5977+
Update.newBuilder()
5978+
.namespace(namespace1)
5979+
.table(TABLE_1)
5980+
.partitionKey(Key.ofInt(ACCOUNT_ID, 0))
5981+
.clusteringKey(Key.ofInt(ACCOUNT_TYPE, 0))
5982+
.intValue(BALANCE, 200)
5983+
.build());
5984+
5985+
// Retrieve the record again, but use a different Get object (with a where clause)
5986+
result =
5987+
transaction.get(
5988+
Get.newBuilder()
5989+
.namespace(namespace1)
5990+
.table(TABLE_1)
5991+
.partitionKey(Key.ofInt(ACCOUNT_ID, 0))
5992+
.clusteringKey(Key.ofInt(ACCOUNT_TYPE, 0))
5993+
.where(column(BALANCE).isEqualToInt(200))
5994+
.build());
5995+
5996+
assertThat(result).isNotPresent();
5997+
5998+
assertThatThrownBy(transaction::commit).isInstanceOf(CommitConflictException.class);
5999+
transaction.rollback();
6000+
6001+
// Assert
6002+
result =
6003+
manager.get(
6004+
Get.newBuilder()
6005+
.namespace(namespace1)
6006+
.table(TABLE_1)
6007+
.partitionKey(Key.ofInt(ACCOUNT_ID, 0))
6008+
.clusteringKey(Key.ofInt(ACCOUNT_TYPE, 0))
6009+
.build());
6010+
6011+
assertThat(result).isPresent();
6012+
assertThat(result.get().getInt(ACCOUNT_ID)).isEqualTo(0);
6013+
assertThat(result.get().getInt(ACCOUNT_TYPE)).isEqualTo(0);
6014+
assertThat(result.get().getInt(BALANCE)).isEqualTo(200);
6015+
}
6016+
59306017
@Test
59316018
public void manager_get_GetGivenForCommittedRecord_WithSerializable_ShouldReturnRecord()
59326019
throws TransactionException {

0 commit comments

Comments
 (0)