diff --git a/db/change_cache.go b/db/change_cache.go index 4a906bbd13..ee1583342a 100644 --- a/db/change_cache.go +++ b/db/change_cache.go @@ -503,7 +503,7 @@ func (c *changeCache) DocChanged(event sgbucket.FeedEvent, docType DocumentType) // Now add the entry for the new doc revision: if len(rawUserXattr) > 0 { - collection.revisionCache.RemoveWithRev(ctx, docID, syncData.CurrentRev) + collection.revisionCache.RemoveRevOnly(ctx, docID, syncData.CurrentRev) } change := &LogEntry{ diff --git a/db/crud.go b/db/crud.go index e974e6fe81..a2318b59e8 100644 --- a/db/crud.go +++ b/db/crud.go @@ -2546,7 +2546,7 @@ func (db *DatabaseCollectionWithUser) updateAndReturnDoc(ctx context.Context, do // Prior to saving doc, remove the revision in cache if createNewRevIDSkipped { - db.revisionCache.RemoveWithRev(ctx, doc.ID, doc.CurrentRev) + db.revisionCache.RemoveRevOnly(ctx, doc.ID, doc.CurrentRev) } base.DebugfCtx(ctx, base.KeyCRUD, "Saving doc (seq: #%d, id: %v rev: %v)", doc.Sequence, base.UD(doc.ID), doc.CurrentRev) diff --git a/db/revision_cache_bypass.go b/db/revision_cache_bypass.go index 38fcc20b60..03da3334aa 100644 --- a/db/revision_cache_bypass.go +++ b/db/revision_cache_bypass.go @@ -128,6 +128,10 @@ func (rc *BypassRevisionCache) RemoveWithRev(ctx context.Context, docID, revID s // no-op } +func (rc *BypassRevisionCache) RemoveRevOnly(ctx context.Context, docID, revID string, collectionID uint32) { + // no-op +} + func (rc *BypassRevisionCache) RemoveWithCV(ctx context.Context, docID string, cv *Version, collectionID uint32) { // no-op } diff --git a/db/revision_cache_interface.go b/db/revision_cache_interface.go index 37af48da8f..62df969c69 100644 --- a/db/revision_cache_interface.go +++ b/db/revision_cache_interface.go @@ -55,6 +55,8 @@ type RevisionCache interface { // RemoveWithCV evicts a revision from the cache using its current version. RemoveWithCV(ctx context.Context, docID string, cv *Version, collectionID uint32) + RemoveRevOnly(ctx context.Context, docID, revID string, collectionID uint32) + // UpdateDelta stores the given toDelta value in the given rev if cached UpdateDelta(ctx context.Context, docID, revID string, collectionID uint32, toDelta RevisionDelta) @@ -172,6 +174,10 @@ func (c *collectionRevisionCache) RemoveWithRev(ctx context.Context, docID, revI (*c.revCache).RemoveWithRev(ctx, docID, revID, c.collectionID) } +func (c *collectionRevisionCache) RemoveRevOnly(ctx context.Context, docID, revID string) { + (*c.revCache).RemoveRevOnly(ctx, docID, revID, c.collectionID) +} + // RemoveWithCV is for per collection access to Remove method func (c *collectionRevisionCache) RemoveWithCV(ctx context.Context, docID string, cv *Version) { (*c.revCache).RemoveWithCV(ctx, docID, cv, c.collectionID) diff --git a/db/revision_cache_lru.go b/db/revision_cache_lru.go index dc3336d083..8b8ee59d4a 100644 --- a/db/revision_cache_lru.go +++ b/db/revision_cache_lru.go @@ -93,6 +93,10 @@ func (sc *ShardedLRURevisionCache) RemoveWithCV(ctx context.Context, docID strin sc.getShard(docID).RemoveWithCV(ctx, docID, cv, collectionID) } +func (sc *ShardedLRURevisionCache) RemoveRevOnly(ctx context.Context, docID, revID string, collectionID uint32) { + sc.getShard(docID).RemoveRevOnly(ctx, docID, revID, collectionID) +} + // An LRU cache of document revision bodies, together with their channel access. type LRURevisionCache struct { backingStores map[uint32]RevisionCacheBackingStore @@ -507,6 +511,12 @@ func (rc *LRURevisionCache) RemoveWithCV(ctx context.Context, docID string, cv * rc.removeFromCacheByCV(ctx, docID, cv, collectionID) } +// RemoveRevOnly removes a rev from revision cache lookup map, if present. +func (rc *LRURevisionCache) RemoveRevOnly(ctx context.Context, docID, revID string, collectionID uint32) { + // This will only remove the entry from the rev lookup map, not the lru list + rc.removeFromRevLookup(ctx, docID, revID, collectionID) +} + // removeFromCacheByCV removes an entry from rev cache by CV func (rc *LRURevisionCache) removeFromCacheByCV(ctx context.Context, docID string, cv *Version, collectionID uint32) { key := IDandCV{DocID: docID, Source: cv.SourceID, Version: cv.Value, CollectionID: collectionID} @@ -518,6 +528,7 @@ func (rc *LRURevisionCache) removeFromCacheByCV(ctx context.Context, docID strin } // grab the revid key from the value to enable us to remove the reference from the rev lookup map too elem := element.Value.(*revCacheValue) + legacyKey := IDAndRev{DocID: docID, RevID: elem.revID, CollectionID: collectionID} rc.lruList.Remove(element) delete(rc.hlvCache, key) @@ -538,6 +549,7 @@ func (rc *LRURevisionCache) removeFromCacheByRev(ctx context.Context, docID, rev } // grab the cv key from the value to enable us to remove the reference from the rev lookup map too elem := element.Value.(*revCacheValue) + hlvKey := IDandCV{DocID: docID, Source: elem.cv.SourceID, Version: elem.cv.Value, CollectionID: collectionID} rc.lruList.Remove(element) // decrement the overall memory bytes count @@ -548,6 +560,16 @@ func (rc *LRURevisionCache) removeFromCacheByRev(ctx context.Context, docID, rev rc.cacheNumItems.Add(-1) } +// removeFromRevLookup will only remove the entry from the rev lookup map, if present. Underlying element must stay in list for eviction to work. +func (rc *LRURevisionCache) removeFromRevLookup(ctx context.Context, docID, revID string, collectionID uint32) { + key := IDAndRev{DocID: docID, RevID: revID, CollectionID: collectionID} + rc.lock.Lock() + defer rc.lock.Unlock() + // only delete from rev lookup map, if we delete underlying element in list, the now elem in the HLV lookup map + // will never be evicted leading to potential unbounded growth of HLV lookup map + delete(rc.cache, key) +} + // removeValue removes a value from the revision cache, if present and the value matches the the value. If there's an item in the revision cache with a matching docID and revID but the document is different, this item will not be removed from the rev cache. func (rc *LRURevisionCache) removeValue(value *revCacheValue) { rc.lock.Lock() @@ -583,8 +605,17 @@ func (rc *LRURevisionCache) _numberCapacityEviction() (numItemsEvicted int64, nu } hlvKey := IDandCV{DocID: value.id, Source: value.cv.SourceID, Version: value.cv.Value, CollectionID: value.collectionID} revKey := IDAndRev{DocID: value.id, RevID: value.revID, CollectionID: value.collectionID} - delete(rc.cache, revKey) delete(rc.hlvCache, hlvKey) + if elem := rc.cache[revKey]; elem != nil { + revValue := elem.Value.(*revCacheValue) + // we need to check if the value pointed to by the rev lookup map is the same value we're evicting, this is + // because we can have can currently have two items with the same docID and revID, but different CVs due to + // a new HLV being generated for user xattr updates where we don't generate a new revID. + if revValue.cv.String() == value.cv.String() { + // this rev lookup item matches the value we're evicting, so remove it + delete(rc.cache, revKey) + } + } numItemsEvicted++ numBytesEvicted += value.getItemBytes() } @@ -832,8 +863,17 @@ func (rc *LRURevisionCache) performEviction(ctx context.Context) { } revKey := IDAndRev{DocID: value.id, RevID: value.revID, CollectionID: value.collectionID} hlvKey := IDandCV{DocID: value.id, Source: value.cv.SourceID, Version: value.cv.Value, CollectionID: value.collectionID} - delete(rc.cache, revKey) delete(rc.hlvCache, hlvKey) + if elem := rc.cache[revKey]; elem != nil { + revValue := elem.Value.(*revCacheValue) + // we need to check if the value pointed to by the rev lookup map is the same value we're evicting, this is + // because we can have can currently have two items with the same docID and revID, but different CVs due to + // a new HLV being generated for user xattr updates where we don't generate a new revID. + if revValue.cv.String() == value.cv.String() { + // this rev lookup item matches the value we're evicting, so remove it + delete(rc.cache, revKey) + } + } numItemsRemoved++ valueBytes := value.getItemBytes() numBytesRemoved += valueBytes diff --git a/db/revision_cache_test.go b/db/revision_cache_test.go index d6b4c35a01..9e6dadfd0f 100644 --- a/db/revision_cache_test.go +++ b/db/revision_cache_test.go @@ -2239,3 +2239,55 @@ func TestRevCacheOnDemandImportNoCache(t *testing.T) { _, exists = collection.revisionCache.Peek(ctx, docID, doc.CurrentRev) require.False(t, exists) } + +func TestRemoveFromRevLookup(t *testing.T) { + cacheHitCounter, cacheMissCounter, cacheNumItems, memoryBytesCounted := base.SgwIntStat{}, base.SgwIntStat{}, base.SgwIntStat{}, base.SgwIntStat{} + backingStoreMap := CreateTestSingleBackingStoreMap(&noopBackingStore{}, testCollectionID) + cacheOptions := &RevisionCacheOptions{ + MaxItemCount: 10, + MaxBytes: 0, + } + cache := NewLRURevisionCache(cacheOptions, backingStoreMap, &cacheHitCounter, &cacheMissCounter, &cacheNumItems, &memoryBytesCounted) + + ctx := base.TestCtx(t) + + // Fill up the rev cache with the first 10 docs + for docID := 0; docID < 10; docID++ { + id := strconv.Itoa(docID) + vrs := uint64(docID) + cache.Put(ctx, DocumentRevision{BodyBytes: []byte(`{}`), DocID: id, RevID: "1-abc", CV: &Version{Value: vrs, SourceID: "test"}, History: Revisions{"start": 1}}, testCollectionID) + } + assert.Equal(t, int64(10), cacheNumItems.Value()) + assert.Equal(t, int64(20), memoryBytesCounted.Value()) + assert.Equal(t, 10, len(cache.cache)) + assert.Equal(t, 10, len(cache.hlvCache)) + + // simulate a user xattr update: + // 1. Removes form rev lookup cache + // 2. Enter new entry with same revID and docID but diff CV + // 3. Assert that eviction eventually aligns the cache items in each lookup map + cache.RemoveRevOnly(ctx, "1", "1-abc", testCollectionID) + assert.Equal(t, int64(10), cacheNumItems.Value()) + assert.Equal(t, int64(20), memoryBytesCounted.Value()) + assert.Equal(t, 10, cache.lruList.Len()) + assert.Equal(t, 9, len(cache.cache)) + assert.Equal(t, 10, len(cache.hlvCache)) + + // add new entry to cache for docID 1 with a different CV but same revID + cache.Put(ctx, DocumentRevision{BodyBytes: []byte(`{}`), DocID: "1", RevID: "1-abc", CV: &Version{Value: 1234, SourceID: "test"}, History: Revisions{"start": 1}}, testCollectionID) + assert.Equal(t, int64(10), cacheNumItems.Value()) + assert.Equal(t, int64(20), memoryBytesCounted.Value()) + assert.Equal(t, 10, cache.lruList.Len()) + assert.Equal(t, 9, len(cache.cache)) // we should have 9 items in the rev lookup as we removed one item above + assert.Equal(t, 10, len(cache.hlvCache)) + + // add new doc to trigger eviction of old doc "1" value in the cache that has entry in hlv cache to an item but the revID lookup + // for this item will be to a different value (given the simulation of user xattr update above). This means that this + // will trigger an eviction from CV lookup but no revID lookup thus aligning the lookup maps once again. + cache.Put(ctx, DocumentRevision{BodyBytes: []byte(`{}`), DocID: "someNewDoc", RevID: "1-abc", CV: &Version{Value: 123456, SourceID: "test"}, History: Revisions{"start": 1}}, testCollectionID) + assert.Equal(t, int64(10), cacheNumItems.Value()) + assert.Equal(t, int64(20), memoryBytesCounted.Value()) + assert.Equal(t, 10, cache.lruList.Len()) + assert.Equal(t, 10, len(cache.cache)) // we should now have 10 items in rev lookup given the item above aligned the lookups after eviction + assert.Equal(t, 10, len(cache.hlvCache)) +}