CBG-3690 don't re-read document for resync #7892

torcolvin · 2025-11-19T22:14:08Z

CBG-3690 don't re-read document for resync

one fix to create hlv if none exists from CBG-5017 Resync: allow legacy rev documents to be resynced #7890
remove non xattr code pathway
removed redundant args from resyncDocument and unused return param

Pre-review checklist

Removed debug logging (fmt.Print, log.Print, ...)
Logging sensitive data? Make sure it's tagged (e.g. base.UD(docID), base.MD(dbName))
Updated relevant information in the API specifications (such as endpoint descriptions, schemas, ...) in docs/api

Integration Tests

GSI=true,xattrs=true https://jenkins.sgwdev.com/job/SyncGatewayIntegration/171/ (unrelated flake in TestRemoveIndexesUseViewsTrueAndFalse)

Copilot

Pull Request Overview

This pull request optimizes the document resync operation by eliminating unnecessary document re-reads from the bucket. The key changes improve performance by passing the document data directly from DCP feed events to the resync function, and add support for creating HLV (Hybrid Logical Vector) metadata during resync operations.

Key Changes

Modified resyncDocument to accept a *sgbucket.BucketDocument parameter instead of re-reading from the bucket
Added HLV creation logic to the resync path for documents that don't have HLV
Removed the non-xattr code pathway from resync operations

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
db/database.go	Updated `resyncDocument` signature to accept bucket document directly; added HLV update logic during resync
db/background_mgr_resync_dcp.go	Modified DCP callback to convert feed events to bucket documents and pass to resync
db/crud.go	Extended HLV update logic to handle `Resync` case alongside `Import`
db/document.go	Added `bucketDocumentFromFeed` and `getBucketDocument` helper functions; removed unused `UnmarshalDocumentFromFeed`
db/document_test.go	Added test for `getBucketDocument` function
db/database_test.go	Enhanced resync tests to verify HLV creation and metadata-only updates
base/constants.go	Added `VirtualExpiry` constant for fetching document expiry

db/database.go

db/database_test.go

torcolvin · 2025-11-20T14:40:43Z

When looking at this PR, evaluate this for safety to put into 4.0.2 to address CBG-5017 and obviate the need for #7890

db/database.go

torcolvin · 2025-11-26T13:42:17Z

To consider:

Is the latest set of changes better than https://github.com/couchbase/sync_gateway/pull/7892/files/eb809bf36ded144a008de3741c629a11a999d827 ? Does it add more risk to the ticket to make it not worthwhile? The idea behind the latest set of changes is to avoid having to set mouMatch and moving the update to a single point in code. This means that we move

The refactor exposes something that is tricky which is that Document.RevSeqNo is present on the Document struct but only populated when an import is occurring, since we do not want to fetch an extra xattr if needed. This means the value is often 0, so I added code to check if Document.RevSeqNo = 0 and not write the mou in this case. An error here would be a developer problem.

I thought about if the right thing to do is to add RevSeqNo to sgbucket.BucketDocument which almost fixes the problem, except for on of the on demand import fields on write. Because documentUpdateFunc takes db.Document object, we need to be able to pass rev seq no in that function. That refactor became even bigger than the existing refactor that I did to consolidate mou handling.

torcolvin · 2025-12-02T02:00:08Z

To consider:

Is the latest set of changes better than couchbase/sync_gateway/pull/7892/files/eb809bf36ded144a008de3741c629a11a999d827 ? Does it add more risk to the ticket to make it not worthwhile? The idea behind the latest set of changes is to avoid having to set mouMatch and moving the update to a single point in code. This means that we move

The refactor exposes something that is tricky which is that Document.RevSeqNo is present on the Document struct but only populated when an import is occurring, since we do not want to fetch an extra xattr if needed. This means the value is often 0, so I added code to check if Document.RevSeqNo = 0 and not write the mou in this case. An error here would be a developer problem.

I thought about if the right thing to do is to add RevSeqNo to sgbucket.BucketDocument which almost fixes the problem, except for on of the on demand import fields on write. Because documentUpdateFunc takes db.Document object, we need to be able to pass rev seq no in that function. That refactor became even bigger than the existing refactor that I did to consolidate mou handling.

This entire comment is obviated since it was determined to always update _mou and never _vv in 693e4b5

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

db/import_test.go

db/database_collection.go

base/constants.go

adamcfraser

Generally looks good, a few questions on the details.

adamcfraser · 2025-11-20T15:18:08Z

db/document.go

+	}, nil
+}
+
+func getBucketDocument(ctx context.Context, collection *DatabaseCollection, docID string) (*sgbucket.BucketDocument, error) {


Is this a test-only function at this point in time? I was going to suggest that import could rely on preserveExpiry at this point, but it looks like this function doesn't have any non-test usage?

Since I moved this to test only code, I was going to just leave as is.

adamcfraser · 2025-11-20T15:30:42Z

db/database.go

-			}
-		})
+		if rawGlobalXattr != nil {
+			updatedDoc.Xattrs[base.GlobalXattrName] = rawGlobalXattr


Could this be removed, since resync won't be adding/removing attachments?

adamcfraser · 2025-12-08T21:45:46Z

db/document.go

+		Xattrs:      xattrs,
+		Cas:         event.Cas,
+		Expiry:      event.Expiry,
+		IsTombstone: event.Opcode == sgbucket.FeedOpDeletion,


Does this handle all tombstone cases? e.g. if we modify a system xattr on a document that's already a tombstone, will that be sent as FeedOpDeletion or FeedOpMutation?

Yes, any tombstone mutation will show up as FeedOpDeletion.

adamcfraser · 2025-12-08T21:56:47Z

db/database.go

-				if updatedExpiry != nil {
-					updatedDoc.UpdateExpiry(*updatedExpiry)
-				}
+		updatedDoc, shouldUpdate, updatedExpiry, _, updatedUnusedSequences, err = db.getResyncedDocument(ctx, doc, regenerateSequences, nil)


Did you consider removing highSeq from getResyncedDocument since it doesn't appear to be used? Or maybe a follow-up PR to remove it?

I did decide to remove this.

adamcfraser · 2025-12-08T21:59:18Z

db/database.go

+			},
+			Expiry: updatedExpiry,
+		}
+		if doc.MetadataOnlyUpdate != nil {


Should line 1861 (adding the mou xattr) be inside this if clause? Or is there a scenario where rawMouXattr is non-nil, but doc.MetadataOnlyUpdate is nil?

This if statement doesn't need to exist anymore, it did for <4.0 since Sync Gateway didn't write _mou unless CBS was >=7.6

Copilot AI review requested due to automatic review settings November 19, 2025 22:14

Copilot AI reviewed Nov 19, 2025

View reviewed changes

db/database.go Outdated Show resolved Hide resolved

db/database_test.go Outdated Show resolved Hide resolved

torcolvin mentioned this pull request Nov 20, 2025

CBG-5017 Resync: allow legacy rev documents to be resynced #7890

Merged

4 tasks

torcolvin assigned gregns1 Nov 20, 2025

torcolvin requested a review from gregns1 November 20, 2025 14:40

gregns1 reviewed Nov 21, 2025

View reviewed changes

db/database.go Show resolved Hide resolved

db/database.go Outdated Show resolved Hide resolved

db/database.go Outdated Show resolved Hide resolved

torcolvin assigned torcolvin and adamcfraser and unassigned gregns1 and torcolvin Nov 24, 2025

CBG-3690 do not re-read document for resync

b85e507

torcolvin force-pushed the CBG-3690-fix2 branch from 5081439 to b85e507 Compare December 2, 2025 01:37

remove print

7047360

torcolvin requested a review from Copilot December 4, 2025 14:00

Copilot AI reviewed Dec 4, 2025

View reviewed changes

db/import_test.go Show resolved Hide resolved

db/database_collection.go Outdated Show resolved Hide resolved

base/constants.go Outdated Show resolved Hide resolved

copilot suggestions

f54a069

torcolvin force-pushed the CBG-3690-fix2 branch from 8f27379 to f54a069 Compare December 4, 2025 14:29

adamcfraser requested changes Dec 8, 2025

View reviewed changes

Simplify implementation

a5ab30a

CBG-3690 don't re-read document for resync #7892

Are you sure you want to change the base?

CBG-3690 don't re-read document for resync #7892

Conversation

torcolvin commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-review checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Key Changes

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

torcolvin commented Nov 20, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

torcolvin commented Nov 26, 2025

Uh oh!

torcolvin commented Dec 2, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adamcfraser left a comment

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!

4 participants

torcolvin commented Nov 19, 2025 •

edited

Loading