Uploaded image for project: 'Couchbase Server'
  1. Couchbase Server
  2. MB-40467

VBucket::handlePreExpiry may keep the body in the payload

    XMLWordPrintable

    Details

    • Triage:
      Triaged
    • Story Points:
      1
    • Is this a Regression?:
      No

      Description

      Using a 2 node cluster against 6.6.0 build 7873.

      When running the gocb test suite at some point during the TestQuery test the SDK connection is closed by the server and then all kv tests thereafter fail with temp fail. Looking in the server logs (attached) it looks like memcached crashes and doesn't recover. In the UI the bucket shows as "1 node pending".

      To repro create a 2 node cluster with kv,index,n1ql on one node and kv,index,n1ql,fts,cbas on the other. Create a default bucket. Clone https://github.com/couchbase/gocb and run tests with

      go test -race -v ./ --server <address> --user <user> --pass <password> --bucket default --version 6.5.0

      During the TestQuery test the logs will output an EOF and then the SDK will be unable to recover (and tests will take a long time to complete) due to memcached being down on one node. Unfortunately I've been unable to distill this down into a smaller repro case and I can't tell what is causing the crash. I can reliably reproduce this via our SDK jenkins and locally using our cbdyncluster infrastructure. This was not happening on build 7692 and does not happen against 6.5.1.

        Attachments

          Issue Links

          No reviews matched the request. Check your Options in the drop-down menu of this sections header.

            Activity

            Hide
            pvarley Patrick Varley added a comment -

            Previous version of Couchbase Server, would incorrect kept the value of a document on expiry. When cbbackupmgr backs up the cluster, dcp would sends these tombstones including the value. cbbackupmgr stores this tombstone including the value. When the backup is restore to a cluster before 6.6.0, it would restore the tombstones fine. As it would accepted a delete_with_meta with a value. In 6.6.0 the validator for delete_with_meta was update and it's no longer accepts a delete_with_meta with a value. As a result the restore will fail for those tombstones.

            Show
            pvarley Patrick Varley added a comment - Previous version of Couchbase Server, would incorrect kept the value of a document on expiry. When cbbackupmgr backs up the cluster, dcp would sends these tombstones including the value. cbbackupmgr stores this tombstone including the value. When the backup is restore to a cluster before 6.6.0, it would restore the tombstones fine. As it would accepted a delete_with_meta with a value. In 6.6.0 the validator for delete_with_meta was update and it's no longer accepts a delete_with_meta with a value. As a result the restore will fail for those tombstones.
            Hide
            drigby Dave Rigby added a comment -

            Previous version of Couchbase Server, would incorrect kept the value of a document on expiry.

            What's the MB for this issue?

            Show
            drigby Dave Rigby added a comment - Previous version of Couchbase Server, would incorrect kept the value of a document on expiry. What's the MB for this issue?
            Hide
            paolo.cocchi Paolo Cocchi added a comment - - edited

            Apologies for replying only now, for some reason yesterday I had an outage of mail notifications.

            As per my old comments, this is a longstanding issue that we are seeing only in 6.6.0 because of (or thanks to I would say ) the extra validation that we have added in 6.6.0.
            But, the extra validation is only at DcpConsumer::delete (see http://review.couchbase.org/c/kv_engine/+/127038/8/engines/ep/src/dcp/consumer.cc).
            We did put in some changes at EPE::deleteWithMeta (now it accepts UserXattrs in the payload). But the validation has not changed with regard to document's Body, DelWithMeta would fail also in pre-6.6 (see http://review.couchbase.org/c/kv_engine/+/129191/8/engines/ep/src/ep_engine.cc#5477).

            Show
            paolo.cocchi Paolo Cocchi added a comment - - edited Apologies for replying only now, for some reason yesterday I had an outage of mail notifications. As per my old comments, this is a longstanding issue that we are seeing only in 6.6.0 because of (or thanks to I would say ) the extra validation that we have added in 6.6.0. But, the extra validation is only at DcpConsumer::delete (see http://review.couchbase.org/c/kv_engine/+/127038/8/engines/ep/src/dcp/consumer.cc ). We did put in some changes at EPE::deleteWithMeta (now it accepts UserXattrs in the payload). But the validation has not changed with regard to document's Body, DelWithMeta would fail also in pre-6.6 (see http://review.couchbase.org/c/kv_engine/+/129191/8/engines/ep/src/ep_engine.cc#5477 ).
            Hide
            pvarley Patrick Varley added a comment -

            Opened MB-42352 to look into this further.

            Show
            pvarley Patrick Varley added a comment - Opened MB-42352 to look into this further.
            Hide
            paolo.cocchi Paolo Cocchi added a comment - - edited

            Looked again into this for a full assessment on MB-42352.

            The code that we fixed here could lead to keeping Body and UserXattrs in the payload for an expiration.

            Summary of when we do/don't hit the issue in pre-6.6:
            SysXattrs in the paylaod -> all good (VBucket::handlePreExpiry behaves correctly)
            No SysXattrs:

            • only Body: all good (VBucket::handlePreExpiry generates invalid payloads but VBucket::processExpiredItem rectifies)
            • only UserXattrs: we hit the issue
            • Body + UserXattrs: we hit the issue

             

            Details
            In the specific function that I fixed (VBucket::handlePreExpiry), whether we hit the issue or not depends entirely on the of SysXattrs in the payload:

            • we have SysXattrs -> the function behaves correctly, Body and UserXattr removed if any
            • we don't have SysXattrs -> the function incorrectly leaves the value untouched, so Body and UserXattrs are not removed

            That said, the final behaviour of the full call-stack is that we end up with invalid payloads in expirations iff we don't have SysXattrs but we do have UserXattrs in the value.

            That is because VBucket::processExpiredItem (executed after VBucket::handlePreExpiry) reset the value depending on if datatype:xattr:

            HashTable::FindResult VBucket::fetchValidValue(
                    WantsDeleted wantsDeleted,
                    TrackReference trackReference,
                    QueueExpired queueExpired,
                    const Collections::VB::Manifest::CachingReadHandle& cHandle,
                    const ForGetReplicaOp fetchRequestedForReplicaItem) {
            ..
                        // queueDirty only allowed on active VB
                        if (queueExpired == QueueExpired::Yes &&
                            getState() == vbucket_state_active) {
                            incExpirationStat(ExpireBy::Access);
                            handlePreExpiry(res.lock, *v);    <-    IF PAYLOAD CONTAINS ONLY THE BODY, THEN WE MISS TO REMOVE THE BODY
                            VBNotifyCtx notifyCtx;
                            std::tie(std::ignore, v, notifyCtx) =
                                    processExpiredItem(res.lock, *v, cHandle);    <-    BUT, IF (!xattr) THEN reset the value, SO BODY CORRECTLY REMOVED
                            notifyNewSeqno(notifyCtx);
                            doCollectionsStats(cHandle, notifyCtx);
                        }
            ..
            }
            

            VBucket::processExpiredItem(
                    const HashTable::HashBucketLock& hbl,
                    StoredValue& v,
                    const Collections::VB::Manifest::CachingReadHandle& cHandle) {
            ..
                /* If the datatype is XATTR, mark the item as deleted
                 * but don't delete the value as system xattrs can
                 * still be queried by mobile clients even after
                 * deletion.
                 * TODO: The current implementation is inefficient
                 * but functionally correct and for performance reasons
                 * only the system xattrs need to be stored.
                 */
                value_t value = v.getValue();
                bool onlyMarkDeleted = value && mcbp::datatype::is_xattr(v.getDatatype());    <-    onlyMarkDeleted=false IF PAYLOAD CONTAINS ONLY THE BODY
                v.setRevSeqno(v.getRevSeqno() + 1);
                VBNotifyCtx notifyCtx;
                StoredValue* newSv;
                DeletionStatus delStatus;
                std::tie(newSv, delStatus, notifyCtx) =
                        softDeleteStoredValue(hbl,
                                              v,
                                              onlyMarkDeleted,
                                              VBQueueItemCtx{},
                                              v.getBySeqno(),
                                              DeleteSource::TTL);
            

            We end up in:

            HashTable::DeleteResult HashTable::unlocked_softDelete(
                    const HashBucketLock& hbl,
                    StoredValue& v,
                    bool onlyMarkDeleted,
                    DeleteSource delSource) {
            ..
                case CommittedState::Pending:
                case CommittedState::PreparedMaybeVisible:
                case CommittedState::CommittedViaMutation:
                case CommittedState::CommittedViaPrepare:
                    const auto preProps = valueStats.prologue(&v);
             
                    if (onlyMarkDeleted) {
                        v.markDeleted(delSource);
                    } else {
                        v.del(delSource);    <-    HERE
                        // As part of deleting, set committedState to CommittedViaMutation -
                        // this is necessary so when we later queue this SV into
                        // CheckpoitnManager, if if was previously CommittedViaPrepare it
                        // isn't mis-interpreted for a SyncDelete.
                        v.setCommitted(CommittedState::CommittedViaMutation);
                    }
            ..
            }
            

            And then:

            bool StoredValue::deleteImpl(DeleteSource delSource) {
                if (isDeleted() && !getValue()) {
                    // SV is already marked as deleted and has no value - no further
                    // deletion possible.
                    return false;
                }
             
                resetValue();    <-    HERE
                setDatatype(PROTOCOL_BINARY_RAW_BYTES);
                setPendingSeqno();
             
                setDeletedPriv(true);
                setDeletionSource(delSource);
                markDirty();
             
                return true;
            }
            

            Show
            paolo.cocchi Paolo Cocchi added a comment - - edited Looked again into this for a full assessment on MB-42352 . The code that we fixed here could lead to keeping Body and UserXattrs in the payload for an expiration. Summary of when we do/don't hit the issue in pre-6.6: SysXattrs in the paylaod -> all good (VBucket::handlePreExpiry behaves correctly) No SysXattrs: only Body: all good (VBucket::handlePreExpiry generates invalid payloads but VBucket::processExpiredItem rectifies) only UserXattrs: we hit the issue Body + UserXattrs: we hit the issue   Details In the specific function that I fixed (VBucket::handlePreExpiry), whether we hit the issue or not depends entirely on the of SysXattrs in the payload: we have SysXattrs -> the function behaves correctly, Body and UserXattr removed if any we don't have SysXattrs -> the function incorrectly leaves the value untouched, so Body and UserXattrs are not removed That said, the final behaviour of the full call-stack is that we end up with invalid payloads in expirations iff we don't have SysXattrs but we do have UserXattrs in the value. That is because VBucket::processExpiredItem (executed after VBucket::handlePreExpiry) reset the value depending on if datatype:xattr: HashTable::FindResult VBucket::fetchValidValue( WantsDeleted wantsDeleted, TrackReference trackReference, QueueExpired queueExpired, const Collections::VB::Manifest::CachingReadHandle& cHandle, const ForGetReplicaOp fetchRequestedForReplicaItem) { .. // queueDirty only allowed on active VB if (queueExpired == QueueExpired::Yes && getState() == vbucket_state_active) { incExpirationStat(ExpireBy::Access); handlePreExpiry(res.lock, *v); <- IF PAYLOAD CONTAINS ONLY THE BODY, THEN WE MISS TO REMOVE THE BODY VBNotifyCtx notifyCtx; std::tie(std::ignore, v, notifyCtx) = processExpiredItem(res.lock, *v, cHandle); <- BUT, IF (!xattr) THEN reset the value, SO BODY CORRECTLY REMOVED notifyNewSeqno(notifyCtx); doCollectionsStats(cHandle, notifyCtx); } .. } VBucket::processExpiredItem( const HashTable::HashBucketLock& hbl, StoredValue& v, const Collections::VB::Manifest::CachingReadHandle& cHandle) { .. /* If the datatype is XATTR, mark the item as deleted * but don't delete the value as system xattrs can * still be queried by mobile clients even after * deletion. * TODO: The current implementation is inefficient * but functionally correct and for performance reasons * only the system xattrs need to be stored. */ value_t value = v.getValue(); bool onlyMarkDeleted = value && mcbp::datatype::is_xattr(v.getDatatype()); <- onlyMarkDeleted=false IF PAYLOAD CONTAINS ONLY THE BODY v.setRevSeqno(v.getRevSeqno() + 1); VBNotifyCtx notifyCtx; StoredValue* newSv; DeletionStatus delStatus; std::tie(newSv, delStatus, notifyCtx) = softDeleteStoredValue(hbl, v, onlyMarkDeleted, VBQueueItemCtx{}, v.getBySeqno(), DeleteSource::TTL); We end up in: HashTable::DeleteResult HashTable::unlocked_softDelete( const HashBucketLock& hbl, StoredValue& v, bool onlyMarkDeleted, DeleteSource delSource) { .. case CommittedState::Pending: case CommittedState::PreparedMaybeVisible: case CommittedState::CommittedViaMutation: case CommittedState::CommittedViaPrepare: const auto preProps = valueStats.prologue(&v);   if (onlyMarkDeleted) { v.markDeleted(delSource); } else { v.del(delSource); <- HERE // As part of deleting, set committedState to CommittedViaMutation - // this is necessary so when we later queue this SV into // CheckpoitnManager, if if was previously CommittedViaPrepare it // isn't mis-interpreted for a SyncDelete. v.setCommitted(CommittedState::CommittedViaMutation); } .. } And then: bool StoredValue::deleteImpl(DeleteSource delSource) { if (isDeleted() && !getValue()) { // SV is already marked as deleted and has no value - no further // deletion possible. return false; }   resetValue(); <- HERE setDatatype(PROTOCOL_BINARY_RAW_BYTES); setPendingSeqno();   setDeletedPriv(true); setDeletionSource(delSource); markDirty();   return true; }

              People

              Assignee:
              charles.dixon Charles Dixon
              Reporter:
              charles.dixon Charles Dixon
              Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

                Dates

                Due:
                Created:
                Updated:
                Resolved:

                  Gerrit Reviews

                  There are no open Gerrit changes

                    PagerDuty