Change deleteStore to check StoreHeader before bumping MetaDataVersionStamp by ScottDugas · Pull Request #4354 · FoundationDB/fdb-record-layer

ScottDugas · 2026-07-14T19:08:02Z

This changes deleteStore to only bump the MetaDataVersionStamp if it is deleting a store that is cacheable.
By being more restrictive, deleting stores that aren't caching the header means that we won't force a refresh on stores that are using the store header cache. This can be pretty valuable, since you would generally use the store state cache for stores that have a lot of activity, but rarely change store state. Those are unlikely to be deleted, and would be most impacted by a spurious meta-data version-stamp bump.

In particular the catalog uses the MetaDataVersion-based store cache, and when you delete a schema (as part of a test cleanup), the old deleteStore would invalidate the metaDataVersion cache, which would cause any catalog operations (which opened the store) to conflict.

Closes #4335

…nStamp In particular the catalog uses the MetaDataVersion-based store cache, and when you delete a schema (as part of a test cleanup), the old deleteStore would invalidate the metadataversion cache, which would cause any catalog operations (which opened the store) to conflict. By being more restrictive, deleting stores that aren't caching the header means that we won't force a refresh on stores that are using the store header cache. Closes FoundationDB#4335

deleteStore reads STORE_INFO_KEY at snapshot isolation to avoid adding a read conflict on every delete. This leaves a race with a concurrent commit that flips the store to cacheable via setStateCacheability(true) and bumps the meta-data version stamp: our snapshot read misses the flip, we skip our own bump, and the delete lands. Sibling clients that populated their caches from the flipper committed state then keep serving reads out of a header for a store that no longer exists. Fix: only on the branches where we decide NOT to bump (header absent, or header parses as non-cacheable), add an explicit point read conflict on STORE_INFO_KEY. That way: - the common case (delete of a store that was and stays non-cacheable) still contributes zero read conflicts, and - the racy case (concurrent flip to cacheable) is detected: the writer SET on STORE_INFO_KEY overlaps our added read conflict and we lose the commit race. The cacheable-header branch and the invalid-header fallback continue to bump unconditionally without a read conflict -- bumping is safe regardless of what a concurrent writer does. Also updates the javadoc to describe the new semantics. Adds a regression test (concurrentSetCacheabilityAndDeleteDoesNotLoseTheBump) that fails against the pre-fix implementation: it runs the two racing transactions with pinned read versions, forces the writer to commit first, and asserts that if the delete then commits the stamp must have advanced past the writer post-commit value. Closes FoundationDB#4335

A bunch of DRY and removing unnecessary logic. Also have a version of the conflict test with the commits in the other order

ScottDugas · 2026-07-15T20:10:04Z

+        //     STORE_INFO_KEY overlaps our added read conflict and we lose the commit race.
+        final byte[] headerKey = subspace.pack(STORE_INFO_KEY);
+        final byte[] headerBytes = context.asyncToSync(FDBStoreTimer.Waits.WAIT_LOAD_RECORD_STORE_STATE,
+                context.readTransaction(true).get(headerKey));


I'll concede, this may be an unnecessary optimization, and maybe it would be better to just read at serializable, and avoid re-adding the conflict key contingently. But, it keeps it closer to the original conflicting behavior, potentially allowing a deleteStore through.

I've left my thoughts on this elsewhere, and I think it does seem like a bit of a premature optimization, and I'm not sure how much it really buys us, especially given as most of the time, I think we'll need to increase the consistency level anyway

alecgrieser · 2026-07-21T15:42:25Z

     * @param subspace the subspace containing the record store
     */
    @SuppressWarnings("PMD.CloseResource")
    public static void deleteStore(FDBRecordContext context, Subspace subspace) {


This is modifying a method that was previously non-blocking and makes it blocking (as we now have to wait for the read). Should this be split into:

deleteStoreAsync - which returns a future chained off of the read

deleteStore - which wraps deleteStoreAsync in an asyncToSync call

It's a bit awkward, as that refactoring actually leaves us in the same position, which is that old code which may not be expecting a blocking call will be updated to be blocking, but at least there's a way out for a caller who is paying attention. Alternatively, we could remove deleteStore entirely and only provide deleteStoreAsync, and then callers have to explicitly wait on it.

Incidentally, if we do create a deleteStoreAsync, we probably want to rework deleteStore(FDBRecordContext, KeySpacePath) so that it uses path.toSubspaceAsync instead of toSubspace and then chains the deletion off of that future

That's a good callout.
I decided to add a new deleteStoreAsync method, revert the existing ones, and mark them as DEPRECATED. I've updated all of usages inside FRL to use the async version, leaving some tests around for backwards compatibility.

alecgrieser · 2026-07-21T15:45:06Z

+     * {@link #setStateCacheability(boolean)} that flips the store to cacheable: if such a
+     * writer commits before us, our commit fails with a serialization error and the caller
+     * retries with a fresh snapshot that sees the new header and bumps the stamp. In the
+     * common case (delete of a store that was and stays non-cacheable), no read conflict is


I'm not sure I understand this: wouldn't the common case (a store is deleted that was and remains non-cacheable) result in the read conflict range being added? Is this instead trying to say that the additional read conflict range doesn't result in any transactions actually conflicting?

I switched to just read at serializable, so this comment is gone.

alecgrieser · 2026-07-21T15:48:47Z

+     * The header read uses snapshot isolation to avoid adding a read-conflict range on every
+     * delete. Only when the on-disk header says the store is non-cacheable (or is missing) —


I actually think it would probably be fine to have a read conflict range on every delete. The extra read conflict will only result in transactions failing if the store header is modified--that is to say, someone concurrently creates and deletes the same store, or someone concurrently updates the meta-data while we are deleting it. And with the read conflict key being added anyway if the meta-data version is not bumped, I think the only real conflict case we're saving is that a store with a cached store header is updated concurrently with this delete

Yeah, I was trying to stay truer to the original implementation which explicitly opted not to gather this information. I have changed it to use simple reads and not mess with conflict ranges.

alecgrieser · 2026-07-21T15:59:38Z

+        // every delete. The clear below already creates a write conflict on the whole range,
+        // which serializes us against any concurrent writer that lands INSIDE this subspace.


This feels a little muddled. For this to be true, the hypothetical writer has to have read something in the store in their transaction. Which, granted, is enough for essentially any use case except a concurrent store delete. But this is written in a way that almost suggests that FDB will handle write-write conflicts in a way that it doesn't

I switched to just read at serializable, so this comment is gone.

alecgrieser · 2026-07-21T16:44:07Z

+        //     STORE_INFO_KEY overlaps our added read conflict and we lose the commit race.
+        final byte[] headerKey = subspace.pack(STORE_INFO_KEY);
+        final byte[] headerBytes = context.asyncToSync(FDBStoreTimer.Waits.WAIT_LOAD_RECORD_STORE_STATE,
+                context.readTransaction(true).get(headerKey));


I've left my thoughts on this elsewhere, and I think it does seem like a bit of a premature optimization, and I'm not sure how much it really buys us, especially given as most of the time, I think we'll need to increase the consistency level anyway

alecgrieser · 2026-07-21T16:46:09Z

+                shouldBump = false;
+            }
+        }
+        if (shouldBump) {


I think context.ensureActive().addReadConflictKey could be moved out to the else condition here, as any time we don't bump the meta-data version stamp, we need to add the read conflict key. I think it makes some sense, too--we need to have the read conflict key present to protect against someone doing something that relies on the meta-data version stamp being bumped to notice a change. So, if we choose not to bump it, we need to have the read conflict range to prevent us from affecting that change

I switched to just read at serializable, so this section has been simplified.

alecgrieser · 2026-07-21T16:49:17Z

+        //   - the common case (delete of a store that was and stays non-cacheable) still
+        //     contributes zero read conflicts, and


This is I think similarly off/confusing as alluded to here: https://github.com/FoundationDB/fdb-record-layer/pull/4354/changes#r3623567943

I switched to just read at serializable, so this comment is gone.

alecgrieser · 2026-07-21T16:50:13Z

+    /**
+     * Deleting a non-cacheable store must NOT bump the meta-data version stamp: no other
+     * client can possibly hold a cached copy of a non-cacheable header, so the version stamp
+     * (a JVM-wide bottleneck on the {@code \xff/metadataVersion} key) shouldn't be touched.


I'm not sure "JVM-wide" is the best description here, as it's more like cluster-wide

No, JVM-wide is, IIUC misleading and correct.

alecgrieser · 2026-07-21T16:55:41Z

+     * there's no cached header to invalidate.
+     */
+    @Test
+    void deleteMissingStoreDoesNotBumpMetaDataVersionStamp() {


Do we have a case where we have a cacheable store, and we validate that we bump the meta-data version? I also think we could add a test that concurrently deletes a store with a cacheable meta-data and then does something else to it (e.g., bump the meta-data version or mark it as not cacheable or bump the user version), and then the delete should succeed even if the operation is concurrent with it. At least as currently written

Do we have a case where we have a cacheable store, and we validate that we bump the meta-data version?

deleteCacheableStoreBumpsMetaDataVersionStamp, or is that not what you were thinking?

I added the other conflict tests, and then some, and it started to feel like these tests were no longer appropriate in FDBRecordStoreStateCacheTest, so I moved them out into a new DeleteStoreTest, along with moving some shared utilities. This will be a separate commit, so going commit-by-commit may be wise.

This reverts `deleteStore`, and adds `deleteStoreAsync` with the new implementation that does not bump the MetaDataVersionStamp excessively. Additionally it changes the new implementation to just read the store header at serializable isolation, rather than trying to do the trickery where it re-adds the range conflict.

Some of the new additions around delete store, seemed to really be going beyond the cache, so I moved all the delete store tests out. This was pretty mechanical, I just cut+pasted the tests, move the static objects to the utils class, and updated references. I did not move VersionIndexTest.deleteStoreWithUncommittedVersionData because that depended on a lot more inner details of the VersionIndexTest

ScottDugas added 5 commits July 11, 2026 07:49

Simplify tests and inverted version of conflicting one

756a08d

A bunch of DRY and removing unnecessary logic. Also have a version of the conflict test with the commits in the other order

Cleanup comments / assertion messages

5dc8ed3

Slightly more test DRY

0f1eefd

ScottDugas added the enhancement New feature or request label Jul 14, 2026

ScottDugas force-pushed the subissue-4335 branch from ccbe336 to 0f1eefd Compare July 14, 2026 20:16

ScottDugas commented Jul 15, 2026

View reviewed changes

DRY more of the test

a7d32d3

ScottDugas requested a review from alecgrieser July 16, 2026 14:56

ScottDugas marked this pull request as ready for review July 16, 2026 14:56

alecgrieser requested changes Jul 21, 2026

View reviewed changes

ScottDugas added 7 commits July 24, 2026 09:46

Parameterize the some tests to run with old & new deleteStore methods

bdc6f02

Add more coverage of concurrent ops with deleteStore

445fd40

Add basic tests of deleteStore

b32c95b

Remove static imports

e8e89f5

Elaborate on some of the metadata-version key javadocs

34e7e31

		* The header read uses snapshot isolation to avoid adding a read-conflict range on every
		* delete. Only when the on-disk header says the store is non-cacheable (or is missing) —

		// every delete. The clear below already creates a write conflict on the whole range,
		// which serializes us against any concurrent writer that lands INSIDE this subspace.

		// - the common case (delete of a store that was and stays non-cacheable) still
		// contributes zero read conflicts, and

Uh oh!

Conversation

ScottDugas commented Jul 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ScottDugas commented Jul 14, 2026 •

edited

Loading