Skip to content

Commit

Permalink
Resolves #1936: Reduce unnecessary clears of legacy version space
Browse files Browse the repository at this point in the history 
This reduces the number of unnecessary clears issued during check version by being more precise about when the legacy version subspace is cleared. It will only clear the range now if (1) the store is not new and (2) the store header suggests that any versions in the old store would have been stored in the old space. It introduces a test that validates that the version is cleared if versions were stored in that old subspace and the meta-data is changed to so that versions are no longer stores. That test valdiates that there are fewer range clears now if the range doesn't have to be cleared, but other than that assert, the test asserts pass both before and after the main code changes.

This resolves #1936.
  • Loading branch information
@alecgrieser
alecgrieser committed Dec 5, 2022
1 parent a5f98c4 commit 72f3391
Show file tree
Hide file tree
Showing 3 changed files with 99 additions and 11 deletions.
2 changes: 1 addition & 1 deletion docs/ReleaseNotes.md
View file
Expand Up @@ -21,7 +21,7 @@ The Guava dependency version has been updated to 31.1. Projects may need to chec
* **Bug fix** Fix 4 [(Issue #NNN)](https://github.com/FoundationDB/fdb-record-layer/issues/NNN)
* **Bug fix** Fix 5 [(Issue #NNN)](https://github.com/FoundationDB/fdb-record-layer/issues/NNN)
* **Performance** Improvement 1 [(Issue #NNN)](https://github.com/FoundationDB/fdb-record-layer/issues/NNN)
* **Performance** Improvement 2 [(Issue #NNN)](https://github.com/FoundationDB/fdb-record-layer/issues/NNN)
* **Performance** Reduce the number of extraneous clear ranges issued during `checkVersion` [(Issue #1936)](https://github.com/FoundationDB/fdb-record-layer/issues/1936)
* **Performance** Improvement 3 [(Issue #NNN)](https://github.com/FoundationDB/fdb-record-layer/issues/NNN)
* **Performance** Improvement 4 [(Issue #NNN)](https://github.com/FoundationDB/fdb-record-layer/issues/NNN)
* **Performance** Improvement 5 [(Issue #NNN)](https://github.com/FoundationDB/fdb-record-layer/issues/NNN)
Expand Down
15 changes: 11 additions & 4 deletions ...ore/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStore.java
View file
Expand Up @@ -331,10 +331,14 @@ public int getUserVersion() {
}

private boolean useOldVersionFormat() {
return useOldVersionFormat(getFormatVersion(), omitUnsplitRecordSuffix);
}

private static boolean useOldVersionFormat(int formatVersion, boolean omitUnsplitRecordSuffix) {
// If the store is either explicitly using the older format version or if
// it is using a newer one, but because of how the data were originally stored
// in this record store, then use the older location for record versions.
return getFormatVersion() < SAVE_VERSION_WITH_RECORD_FORMAT_VERSION || omitUnsplitRecordSuffix;
return formatVersion < SAVE_VERSION_WITH_RECORD_FORMAT_VERSION || omitUnsplitRecordSuffix;
}

/**
Expand Down Expand Up @@ -2042,11 +2046,11 @@ private CompletableFuture<Boolean> checkVersion(@Nonnull RecordMetaDataProto.Dat
private CompletableFuture<Void> checkUserVersion(@Nullable UserVersionChecker userVersionChecker,
@Nonnull final RecordMetaDataProto.DataStoreInfo storeHeader,
@Nonnull RecordMetaDataProto.DataStoreInfo.Builder info, @Nonnull boolean[] dirty) {
final boolean newStore = info.getFormatVersion() == 0;
final int oldUserVersion = newStore ? -1 : info.getUserVersion();
if (userVersionChecker == null) {
return AsyncUtil.DONE;
}
final boolean newStore = info.getFormatVersion() == 0;
final int oldUserVersion = newStore ? -1 : info.getUserVersion();
return userVersionChecker.checkUserVersion(storeHeader, metaDataProvider)
.thenApply(newUserVersion -> {
userVersion = newUserVersion;
Expand Down Expand Up @@ -3920,7 +3924,10 @@ private CompletableFuture<Void> checkRebuild(@Nullable UserVersionChecker userVe
final boolean metaDataVersionChanged = oldMetaDataVersion != newMetaDataVersion;
if (metaDataVersionChanged) {
// Clear the version table if we are no longer storing record versions.
if (!metaData.isStoreRecordVersions()) {
// This can be skipped if the store is new (in which case there is no data), or if the old
// store did not use the old version format to store record versions
if (!metaData.isStoreRecordVersions() && !newStore
&& useOldVersionFormat(oldFormatVersion, omitUnsplitRecordSuffix)) {
final Transaction tr = ensureContextActive();
tr.clear(getSubspace().subspace(Tuple.from(RECORD_VERSION_KEY)).range());
}
Expand Down
93 changes: 87 additions & 6 deletions ...st/java/com/apple/foundationdb/record/provider/foundationdb/indexes/VersionIndexTest.java
View file
Expand Up @@ -22,6 +22,7 @@

import com.apple.foundationdb.record.ExecuteProperties;
import com.apple.foundationdb.record.IndexEntry;
import com.apple.foundationdb.record.IndexFetchMethod;
import com.apple.foundationdb.record.IndexScanType;
import com.apple.foundationdb.record.ObjectPlanHash;
import com.apple.foundationdb.record.PlanHashable;
Expand Down Expand Up @@ -55,10 +56,12 @@
import com.apple.foundationdb.record.provider.foundationdb.FDBQueriedRecord;
import com.apple.foundationdb.record.provider.foundationdb.FDBRecord;
import com.apple.foundationdb.record.provider.foundationdb.FDBRecordContext;
import com.apple.foundationdb.record.provider.foundationdb.FDBRecordContextConfig;
import com.apple.foundationdb.record.provider.foundationdb.FDBRecordStore;
import com.apple.foundationdb.record.provider.foundationdb.FDBRecordStoreBase;
import com.apple.foundationdb.record.provider.foundationdb.FDBRecordStoreTestBase.RecordMetaDataHook;
import com.apple.foundationdb.record.provider.foundationdb.FDBRecordVersion;
import com.apple.foundationdb.record.provider.foundationdb.FDBStoreTimer;
import com.apple.foundationdb.record.provider.foundationdb.FDBStoredRecord;
import com.apple.foundationdb.record.provider.foundationdb.FDBTestBase;
import com.apple.foundationdb.record.provider.foundationdb.IndexOrphanBehavior;
Expand All @@ -68,7 +71,6 @@
import com.apple.foundationdb.record.provider.foundationdb.TestKeySpace;
import com.apple.foundationdb.record.query.RecordQuery;
import com.apple.foundationdb.record.query.expressions.Query;
import com.apple.foundationdb.record.IndexFetchMethod;
import com.apple.foundationdb.record.query.plan.RecordQueryPlanner;
import com.apple.foundationdb.record.query.plan.plans.RecordQueryPlan;
import com.apple.foundationdb.subspace.Subspace;
Expand Down Expand Up @@ -230,15 +232,24 @@ public void setUp() {
metaDataBuilder.addIndex("MySimpleRecord", maxEverVersionIndex);
};

// Provide a combination of format versions relevant to versionstamps along with
// information as to whether large records are split
private static Stream<Arguments> formatVersionArguments() {
private static Stream<Integer> formatVersionsOfInterest() {
return Stream.of(
FDBRecordStore.SAVE_UNSPLIT_WITH_SUFFIX_FORMAT_VERSION - 1,
FDBRecordStore.SAVE_UNSPLIT_WITH_SUFFIX_FORMAT_VERSION,
FDBRecordStore.SAVE_VERSION_WITH_RECORD_FORMAT_VERSION,
FDBRecordStore.MAX_SUPPORTED_FORMAT_VERSION
).flatMap(formatVersion -> Stream.of(Arguments.of(formatVersion, true), Arguments.of(formatVersion, false)));
);
}

private static Stream<Integer> formatVersionsOfInterest(int minVersion) {
return formatVersionsOfInterest().filter(version -> version >= minVersion);
}

// Provide a combination of format versions relevant to versionstamps along with
// information as to whether large records are split
private static Stream<Arguments> formatVersionArguments() {
return formatVersionsOfInterest()
.flatMap(formatVersion -> Stream.of(Arguments.of(formatVersion, true), Arguments.of(formatVersion, false)));
}

// Provide a combination of format versions, split and a remote fetch option
Expand Down Expand Up @@ -423,7 +434,10 @@ private FDBRecordContext openContext(@Nullable RecordMetaDataHook hook) {
RecordMetaDataBuilder metaDataBuilder = RecordMetaData.newBuilder().setRecords(TestRecords1Proto.getDescriptor());
hook.apply(metaDataBuilder);

FDBRecordContext context = fdb.openContext();
FDBRecordContextConfig config = FDBRecordContextConfig.newBuilder()
.setTimer(new FDBStoreTimer())
.build();
FDBRecordContext context = fdb.openContext(config);
recordStore = FDBRecordStore.newBuilder()
.setMetaDataProvider(metaDataBuilder)
.setContext(context)
Expand Down Expand Up @@ -1085,6 +1099,73 @@ public void updateWithinContext(int testFormatVersion, boolean testSplitLongReco
}
}

@SuppressWarnings("unused") // used as parameter source for parameterized test
static Stream<Arguments> updateFormatVersionAndVersionStorage() {
return formatVersionsOfInterest().flatMap(firstFormatVersion ->
formatVersionsOfInterest(firstFormatVersion).flatMap(secondFormatVersion ->
formatVersionsOfInterest(secondFormatVersion).flatMap(thirdFormatVersion ->
Stream.of(false, true).map(testSplitLongRecords -> Arguments.of(firstFormatVersion, secondFormatVersion, thirdFormatVersion, testSplitLongRecords)))));
}

@ParameterizedTest(name = "updateFormatVersionAndVersionStorage[firstFormatVersion={0}, secondFormatVersion={1}, thirdFormatVersion={2}, splitLongRecords={2}]")
@MethodSource
public void updateFormatVersionAndVersionStorage(int firstFormatVersion, int secondFormatVersion, int thirdFormatVersion, boolean testSplitLongRecords) {
formatVersion = firstFormatVersion;
splitLongRecords = testSplitLongRecords;

final RecordMetaDataHook noVersionHook = metaDataBuilder -> {
simpleVersionHook.apply(metaDataBuilder);
metaDataBuilder.removeIndex("globalVersion");
metaDataBuilder.removeIndex("MySimpleRecord$num2-version");
metaDataBuilder.setStoreRecordVersions(false);
};

// Save records with versions
final MySimpleRecord record1 = MySimpleRecord.newBuilder().setRecNo(1066).setNumValue2(42).build();
try (FDBRecordContext context = openContext(simpleVersionHook)) {
recordStore.saveRecord(record1);
context.commit();
}

formatVersion = secondFormatVersion;

FDBRecordVersion version;
boolean inLegacyVersionSpace;
try (FDBRecordContext context = openContext(simpleVersionHook)) {
version = recordStore.loadRecord(Tuple.from(1066L)).getVersion();
assertNotNull(version);
inLegacyVersionSpace = context.ensureActive().getRange(recordStore.getLegacyVersionSubspace().range()).iterator().hasNext();
boolean shouldHaveVersionInOldSpace = (secondFormatVersion < FDBRecordStore.SAVE_VERSION_WITH_RECORD_FORMAT_VERSION)
|| (!testSplitLongRecords && firstFormatVersion < FDBRecordStore.SAVE_UNSPLIT_WITH_SUFFIX_FORMAT_VERSION);
assertEquals(shouldHaveVersionInOldSpace, inLegacyVersionSpace);
context.commit();
}

formatVersion = thirdFormatVersion;

try (FDBRecordContext context = openContext(noVersionHook)) {
FDBRecordVersion newVersion = recordStore.loadRecord(Tuple.from(1066L)).getVersion();
if (inLegacyVersionSpace) {
// We clear out the legacy version space if versions are removed, so this should be null
assertNull(newVersion);
} else {
// We leave the version if it's stored with the record
assertEquals(version, newVersion);
}
// There should be 4 range deletes per former index, plus 1 for the version space, if required.
// This assert may need to change if additional indexes subspaces are created
long rangeDeletes = recordStore.getTimer().getCount(FDBStoreTimer.Counts.RANGE_DELETES);
if (inLegacyVersionSpace) {
assertEquals(9L, rangeDeletes);
} else {
assertEquals(8L, rangeDeletes);
}
// The legacy version space should be empty now, either because it was deleted or because the version was
// never there
assertFalse(context.ensureActive().getRange(recordStore.getLegacyVersionSubspace().range()).iterator().hasNext());
}
}

/**
* Store two records with the same primary key in two record stores. Each one should get its own version.
* This validates that the local version cache is per-record-store.
Expand Down

0 comments on commit 72f3391

Please sign in to comment.