CBG-3895 allow xattrs to be deleted at the same time as they are updated #6791

torcolvin · 2024-04-25T23:55:14Z

couchbase/sg-bucket#120
couchbaselabs/rosmar#34

Changed APIs to support xattrs that are being upserted to include xattrs to be deleted. See sg-bucket API.

validate before gocb that we are never deleting that we are never deleting an xattr on an insert (this will fail on CBS with invalid flags because StoreSemantic is Insert). The failure mode was confusing enough that I thought it would be good to match rosmar handling.
validate that deleted xattrs and updated xattrs are distinct sets

Update StoreSemantic which can only be Replace if xattrs are being deleted, it has to be Insert if the xattrs are being updated on a tombstone.

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

Dependencies (if applicable)

Link upstream PRs
Update Go module dependencies when merged

Integration Tests

GSI=true,xattrs=true https://jenkins.sgwdev.com/job/SyncGateway-Integration/2392/ (CBS 7.6.1), one independent 7.6.1 specific failure
GSI=true,xattrs=true https://jenkins.sgwdev.com/job/SyncGateway-Integration/2393/ (CBS 7.2) (fixed issue on jenkins in followup PR)

- Update StoreSemantic which can only be Replace if xattrs are being deleted, it has to be Insert if the xattrs are being updated on a tombstone

adamcfraser

Some initial comments questions on the non-test code changes, will review the test code in more detail in a subsequent pass.

adamcfraser · 2024-05-22T23:44:10Z

base/collection_xattr.go

@@ -189,12 +190,12 @@ func (c *Collection) SubdocWrite(ctx context.Context, k string, subdocKey string
 }

 // subdocGetBodyAndXattr retrieves the document body and xattrs in a single LookupIn subdoc operation.  Does not require both to exist.
-func (c *Collection) subdocGetBodyAndXattrs(ctx context.Context, k string, xattrKeys []string, fetchBody bool) (rawBody []byte, xattrs map[string][]byte, cas uint64, err error) {
+func (c *Collection) subdocGetBodyAndXattrs(ctx context.Context, k string, xattrKeys []string, fetchBody bool) (isTombstone bool, rawBody []byte, xattrs map[string][]byte, cas uint64, err error) {


What's the scenario where isTombstone is required? (i.e. where the combination of fetchBody=true, rawBody=nil, and err=nil isn't sufficient for the caller to determine isTombstone).
One concern I have is that returning isTombstone=false may be misleading if it's actually not being determined (because fetchBody=false).

I thought that this was required if the body was nil or {} because they unmarshal into nil but the only place this was used was where we did a raw comparison so I removed this.

None of the tests in the base package fail but I think https://jenkins.sgwdev.com/job/SyncGateway-Integration/2448/ TestOnDemandWriteImportReplacingNilDoc might be hitting this issue.

adamcfraser · 2024-05-22T23:50:41Z

base/collection_xattr.go

+	return uint64(result.Cas()), nil
+}
+
+// ressurectWithBodyAndXattrs inserts a document and associated xattrs in a single mutateIn operation.


Suggested change

// ressurectWithBodyAndXattrs inserts a document and associated xattrs in a single mutateIn operation.

// resurrectWithBodyAndXattrs inserts a document and associated xattrs in a single mutateIn operation.

The resurrectWithBodyAndXattrs code ended up identical to insertBodyAndXattrs, unless I'm missing something. Can we remove it?

fixed, I kept the outer function as separate so it was clear when you were doing an insertion or a resurrection conceptually.

It is unfortunate that we can't cas check on ressurection.

adamcfraser · 2024-05-22T23:59:04Z

base/collection_xattr.go

 	options.Internal.DocFlags = gocb.SubdocDocFlagAccessDeleted
 	fillMutateInOptions(ctx, options, opts)

 	result, mutateErr := c.Collection.MutateIn(k, mutateOps, options)
 	if mutateErr != nil {
+		if isKVError(mutateErr, memd.StatusSubDocBadMulti) {
+			mutateErr = fmt.Errorf("%w: %w", ErrXattrNotFound, mutateErr)


Is ErrXattrNotFound actually the only case we get StatusSubDocBadMulti?

Does the 'xattr not found' only apply to attempted deletes, or does it also include xattrs we're updating? (i.e. does this function support inserting new xattrs?)

I don't actually know how we would hit this error, so I've removed this.

This comes from deleteBodyAndXattrs so I'm not sure if it was a copy paste mistake.

adamcfraser · 2024-05-23T00:00:01Z

base/collection_xattr.go

 	}
 	mutateOps = append(mutateOps, gocb.ReplaceSpec("", bytesToRawMessage(v), nil))
 	mutateOps = appendMacroExpansions(mutateOps, opts)

 	options := &gocb.MutateInOptions{
 		Expiry:        CbsExpiryToDuration(exp),
-		StoreSemantic: gocb.StoreSemanticsUpsert,
+		StoreSemantic: gocb.StoreSemanticsReplace,


Same question as above - do the xattrs have to already exist to use StoreSemanticsReplace?

Replace implies that the body has to exist, it does not care about the presence of the xattr. This is covered by the test TestWriteWithXattrs but you should make sure to confirm.

adamcfraser · 2024-05-23T00:01:58Z

base/collection_xattr.go

 		Cas:           gocb.Cas(cas),
 	}
 	fillMutateInOptions(ctx, options, opts)
 	result, mutateErr := c.Collection.MutateIn(k, mutateOps, options)
 	if mutateErr != nil {
+		if errors.Is(mutateErr, gocb.ErrDocumentNotFound) {
+			casMismatchErr := sgbucket.CasMismatchErr{Actual: 0, Expected: cas}


Converting a 'not found' to a 'cas mismatch' seems like it's hiding some information from callers. What's the reason to do this conversion here?

I think this had to do with how I was doing an assert. This raises a CasMismatchErr in rosmar because it is expecting to be able to do an insert where there was no body. But I added the assertion that is requireDocNotFoundOrCasMismatch so I've removed this wrapped.

That said, I think in the future wrapping these types of errors would be a good idea, so we don't have to use IsCasMismatch

adamcfraser · 2024-05-23T00:03:21Z

base/collection_xattr.go

+	}
+	if cas == 0 && len(xattrsToDelete) > 0 {
+		return 0, sgbucket.ErrDeleteXattrOnDocumentInsert
+	}


We don't do this check in other methods that attempt to delete xattrs (like updateXattrs). Is it needed there also?

adamcfraser · 2024-05-23T00:07:49Z

base/collection_xattr_common.go

 		}

 		if writeErr == nil {
 			return casOut, nil
 		}

-		if IsCasMismatch(writeErr) {
+		if previousLoopCas != nil && *previousLoopCas == cas {


What scenario has the potential to cause this loop, or is this strictly defensive?

This was added defensively and allowed me to check error conditions.

We talked about this offline, and it was a good idea to do in the tests.

adamcfraser · 2024-05-23T00:09:17Z

base/collection_xattr_common.go

 		}

 		if writeErr == nil {
 			return casOut, nil
 		}

-		if IsCasMismatch(writeErr) {
+		if previousLoopCas != nil && *previousLoopCas == cas {
+			err := RedactErrorf("Stopping an infinite loop trying to update doc with xattr for key=%s, xattrKeys=%s", UD(k), UD(xattrKeys))


I think this error would be better written as something like "CAS retry triggered, but no change in document CAS detected"

torcolvin requested a review from adamcfraser April 25, 2024 23:55

torcolvin assigned adamcfraser and torcolvin and unassigned adamcfraser Apr 25, 2024

torcolvin force-pushed the CBG-3895 branch 2 times, most recently from 5c43c06 to d77b347 Compare May 14, 2024 21:21

CBG-3895 allow xattrs to be deleted at the same time as mutated

215ecf8

- Update StoreSemantic which can only be Replace if xattrs are being deleted, it has to be Insert if the xattrs are being updated on a tombstone

torcolvin force-pushed the CBG-3895 branch from d77b347 to 215ecf8 Compare May 14, 2024 21:31

torcolvin assigned adamcfraser and unassigned torcolvin May 14, 2024

torcolvin added 2 commits May 14, 2024 20:32

Fix errors caught in integration tests

bca7336

Make sure that tests pass with 7.2

dc258d0

adamcfraser requested changes May 23, 2024

View reviewed changes

adamcfraser assigned torcolvin and unassigned adamcfraser May 23, 2024

simplify code, add test

99223fb

torcolvin assigned adamcfraser and unassigned torcolvin May 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CBG-3895 allow xattrs to be deleted at the same time as they are updated #6791

CBG-3895 allow xattrs to be deleted at the same time as they are updated #6791

torcolvin commented Apr 25, 2024 •

edited

adamcfraser left a comment

adamcfraser May 22, 2024

torcolvin May 23, 2024

adamcfraser May 22, 2024

adamcfraser May 22, 2024

torcolvin May 23, 2024

adamcfraser May 22, 2024

torcolvin May 23, 2024

adamcfraser May 23, 2024

torcolvin May 23, 2024

adamcfraser May 23, 2024

torcolvin May 23, 2024

adamcfraser May 23, 2024

adamcfraser May 23, 2024

torcolvin May 23, 2024

adamcfraser May 23, 2024

torcolvin May 23, 2024

	// ressurectWithBodyAndXattrs inserts a document and associated xattrs in a single mutateIn operation.
	// resurrectWithBodyAndXattrs inserts a document and associated xattrs in a single mutateIn operation.

CBG-3895 allow xattrs to be deleted at the same time as they are updated #6791

Are you sure you want to change the base?

CBG-3895 allow xattrs to be deleted at the same time as they are updated #6791

Conversation

torcolvin commented Apr 25, 2024 • edited

Pre-review checklist

Dependencies (if applicable)

adamcfraser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torcolvin commented Apr 25, 2024 •

edited