SERVER-4781 If in initial replication, allow a non-strict mod applying policy.

7 files changed, 129 insertions, 30 deletions

diff --git a/src/mongo/db/oplog.cpp b/src/mongo/db/oplog.cpp
index 8af1d506c2e..33024612af9 100644
--- a/src/mongo/db/oplog.cpp
+++ b/src/mongo/db/oplog.cpp
@@ -768,8 +768,8 @@ namespace mongo {
                 if( !o.getObjectID(_id) ) {
                     /* No _id.  This will be very slow. */
                     Timer t;
-                    updateObjects(ns, o, o, true, false, false, debug, false,
-                                  QueryPlanSelectionPolicy::idElseNatural() );
+                    updateObjectsForReplication(ns, o, o, true, false, false, debug, false,
+                                                QueryPlanSelectionPolicy::idElseNatural() );
                     if( t.millis() >= 2 ) {
                         RARELY OCCASIONALLY log() << "warning, repl doing slow updates (no _id field) for " << ns << endl;
                     }
@@ -784,8 +784,8 @@ namespace mongo {
                               */
                     BSONObjBuilder b;
                     b.append(_id);
-                    updateObjects(ns, o, b.done(), true, false, false , debug, false,
-                                  QueryPlanSelectionPolicy::idElseNatural() );
+                    updateObjectsForReplication(ns, o, b.done(), true, false, false , debug, false,
+                                                QueryPlanSelectionPolicy::idElseNatural() );
                 }
             }
         }
@@ -799,10 +799,16 @@ namespace mongo {
             OpDebug debug;
             BSONObj updateCriteria = op.getObjectField("o2");
             bool upsert = fields[3].booleanSafe() || convertUpdateToUpsert;
-            UpdateResult ur = updateObjects(ns, o, updateCriteria, upsert, /*multi*/ false,
-                                            /*logop*/ false , debug, /*fromMigrate*/ false,
-                                            QueryPlanSelectionPolicy::idElseNatural() );
-            if( ur.num == 0 ) { 
+            UpdateResult ur = updateObjectsForReplication(ns,
+                                                          o,
+                                                          updateCriteria,
+                                                          upsert,
+                                                          /*multi*/ false,
+                                                          /*logop*/ false,
+                                                          debug,
+                                                          /*fromMigrate*/ false,
+                                                          QueryPlanSelectionPolicy::idElseNatural() );
+            if( ur.num == 0 ) {
                 if( ur.mod ) {
                     if( updateCriteria.nFields() == 1 ) {
                         // was a simple { _id : ... } update criteria
diff --git a/src/mongo/db/ops/update.cpp b/src/mongo/db/ops/update.cpp
index 4324ee28701..2b0e073fc03 100644
--- a/src/mongo/db/ops/update.cpp
+++ b/src/mongo/db/ops/update.cpp
@@ -137,7 +137,8 @@ namespace mongo {
                                  OpDebug& debug,
                                  RemoveSaver* rs,
                                  bool fromMigrate,
-                                 const QueryPlanSelectionPolicy& planPolicy ) {
+                                 const QueryPlanSelectionPolicy& planPolicy,
+                                 bool forReplication ) {
 
         DEBUGUPDATE( "update: " << ns
                      << " update: " << updateobj
@@ -163,10 +164,10 @@ namespace mongo {
             if( d && d->indexBuildInProgress ) {
                 set<string> bgKeys;
                 d->inProgIdx().keyPattern().getFieldNames(bgKeys);
-                mods.reset( new ModSet(updateobj, nsdt->indexKeys(), &bgKeys) );
+                mods.reset( new ModSet(updateobj, nsdt->indexKeys(), &bgKeys, forReplication) );
             }
             else {
-                mods.reset( new ModSet(updateobj, nsdt->indexKeys()) );
+                mods.reset( new ModSet(updateobj, nsdt->indexKeys(), 0, forReplication) );
             }
             modsIsIndexed = mods->isIndexed();
         }
@@ -451,6 +452,18 @@ namespace mongo {
         return UpdateResult( 0 , isOperatorUpdate , 0 , BSONObj() );
     }
 
+    void assertUpdate( const char* ns , const BSONObj& updateobj, const BSONObj& patternOrig ) {
+        uassert( 10155 , "cannot update reserved $ collection", strchr(ns, '$') == 0 );
+        if ( strstr(ns, ".system.") ) {
+            /* dm: it's very important that system.indexes is never updated as IndexDetails
+               has pointers into it */
+            uassert( 10156,
+                     str::stream() << "cannot update system collection: "
+                                   << ns << " q: " << patternOrig << " u: " << updateobj,
+                     legalClientSystemNS( ns , true ) );
+        }
+    }
+
     UpdateResult updateObjects( const char* ns,
                                 const BSONObj& updateobj,
                                 const BSONObj& patternOrig,
@@ -461,13 +474,7 @@ namespace mongo {
                                 bool fromMigrate,
                                 const QueryPlanSelectionPolicy& planPolicy ) {
 
-        uassert( 10155 , "cannot update reserved $ collection", strchr(ns, '$') == 0 );
-        if ( strstr(ns, ".system.") ) {
-            /* dm: it's very important that system.indexes is never updated as IndexDetails has pointers into it */
-            uassert( 10156,
-                     str::stream() << "cannot update system collection: " << ns << " q: " << patternOrig << " u: " << updateobj,
-                     legalClientSystemNS( ns , true ) );
-        }
+        assertUpdate( ns , updateobj , patternOrig );
 
         UpdateResult ur = _updateObjects(false, ns, updateobj, patternOrig,
                                          upsert, multi, logop,
@@ -476,6 +483,25 @@ namespace mongo {
         return ur;
     }
 
+    UpdateResult updateObjectsForReplication( const char* ns,
+                                              const BSONObj& updateobj,
+                                              const BSONObj& patternOrig,
+                                              bool upsert,
+                                              bool multi,
+                                              bool logop ,
+                                              OpDebug& debug,
+                                              bool fromMigrate,
+                                              const QueryPlanSelectionPolicy& planPolicy ) {
+
+        assertUpdate( ns , updateobj , patternOrig );
+
+        UpdateResult ur = _updateObjects(false, ns, updateobj, patternOrig,
+                                         upsert, multi, logop,
+                                         debug, 0, fromMigrate, planPolicy, true /* for replication */ );
+        debug.nupdated = ur.num;
+        return ur;
+    }
+
     BSONObj applyUpdateOperators( const BSONObj& from, const BSONObj& operators ) {
         ModSet mods( operators );
         return mods.prepare( from )->createNewFromMods();
diff --git a/src/mongo/db/ops/update.h b/src/mongo/db/ops/update.h
index 76d864821c8..c24f0628091 100644
--- a/src/mongo/db/ops/update.h
+++ b/src/mongo/db/ops/update.h
@@ -58,6 +58,23 @@ namespace mongo {
                                bool fromMigrate = false,
                                const QueryPlanSelectionPolicy& planPolicy = QueryPlanSelectionPolicy::any());
 
+    /*
+     * Similar to updateObjects but not strict about applying mods that can fail during initial
+     * replication.
+     *
+     * Reference ticket: SERVER-4781
+     */
+    UpdateResult updateObjectsForReplication(const char* ns,
+                                             const BSONObj& updateobj,
+                                             const BSONObj& pattern,
+                                             bool upsert,
+                                             bool multi,
+                                             bool logop,
+                                             OpDebug& debug,
+                                             bool fromMigrate = false,
+                                             const QueryPlanSelectionPolicy& planPolicy =
+                                                 QueryPlanSelectionPolicy::any());
+
     UpdateResult _updateObjects(bool su,
                                 const char* ns,
                                 const BSONObj& updateobj,
@@ -68,7 +85,8 @@ namespace mongo {
                                 OpDebug& debug,
                                 RemoveSaver* rs = 0,
                                 bool fromMigrate = false,
-                                const QueryPlanSelectionPolicy& planPolicy = QueryPlanSelectionPolicy::any());
+                                const QueryPlanSelectionPolicy& planPolicy = QueryPlanSelectionPolicy::any(),
+                                bool forReplication = false);
 
 
     /**
diff --git a/src/mongo/db/ops/update_internal.cpp b/src/mongo/db/ops/update_internal.cpp
index c68a1cac12d..98d323f5db2 100644
--- a/src/mongo/db/ops/update_internal.cpp
+++ b/src/mongo/db/ops/update_internal.cpp
@@ -802,9 +802,23 @@ namespace mongo {
             switch ( cmp ) {
 
             case LEFT_SUBFIELD: { // Mod is embedded under this element
-                uassert( 10145,
-                         str::stream() << "LEFT_SUBFIELD only supports Object: " << field
-                         << " not: " << e.type() , e.type() == Object || e.type() == Array );
+
+                // SERVER-4781
+                bool isObjOrArr = e.type() == Object || e.type() == Array;
+                if ( ! isObjOrArr ) {
+                    if (m->second->m->strictApply) {
+                        uasserted( 10145,
+                                   str::stream() << "LEFT_SUBFIELD only supports Object: " << field
+                                   << " not: " << e.type() );
+                    }
+                    else {
+                        // skip both as we're not applying this mod
+                        e = es.next();
+                        m++;
+                        continue;
+                    }
+                }
+
                 if ( onedownseen.count( e.fieldName() ) == 0 ) {
                     onedownseen.insert( e.fieldName() );
                     if ( e.type() == Object ) {
@@ -933,7 +947,8 @@ namespace mongo {
     ModSet::ModSet(
         const BSONObj& from ,
         const set<string>& idxKeys,
-        const set<string>* backgroundKeys)
+        const set<string>* backgroundKeys,
+        bool forReplication)
         : _isIndexed(0) , _hasDynamicArray( false ) {
 
         BSONObjIterator it(from);
@@ -1022,13 +1037,13 @@ namespace mongo {
                              strstr( target , ".$" ) == 0 );
 
                     Mod from;
-                    from.init( Mod::RENAME_FROM, f );
+                    from.init( Mod::RENAME_FROM, f , forReplication );
                     from.setFieldName( fieldName );
                     updateIsIndexed( from, idxKeys, backgroundKeys );
                     _mods[ from.fieldName ] = from;
 
                     Mod to;
-                    to.init( Mod::RENAME_TO, f );
+                    to.init( Mod::RENAME_TO, f , forReplication );
                     to.setFieldName( target );
                     updateIsIndexed( to, idxKeys, backgroundKeys );
                     _mods[ to.fieldName ] = to;
@@ -1040,7 +1055,7 @@ namespace mongo {
                 _hasDynamicArray = _hasDynamicArray || strstr( fieldName , ".$" ) > 0;
 
                 Mod m;
-                m.init( op , f );
+                m.init( op , f , forReplication );
                 m.setFieldName( f.fieldName() );
                 updateIsIndexed( m, idxKeys, backgroundKeys );
                 _mods[m.fieldName] = m;
diff --git a/src/mongo/db/ops/update_internal.h b/src/mongo/db/ops/update_internal.h
index e2d1d4754ef..68f363f9e1f 100644
--- a/src/mongo/db/ops/update_internal.h
+++ b/src/mongo/db/ops/update_internal.h
@@ -45,13 +45,19 @@ namespace mongo {
         const char* fieldName;
         const char* shortFieldName;
 
+        // Detemines if this mod must absoluetly be applied. In some replication scenarios, a
+        // failed apply of a mod does not constitute an error. In those cases, setting strict
+        // to off would not throw errors.
+        bool strictApply;
+
         BSONElement elt; // x:5 note: this is the actual element from the updateobj
         boost::shared_ptr<Matcher> matcher;
         bool matcherOnPrimitive;
 
-        void init( Op o , BSONElement& e ) {
+        void init( Op o , BSONElement& e , bool forReplication ) {
             op = o;
             elt = e;
+            strictApply = !forReplication;
             if ( op == PULL && e.type() == Object ) {
                 BSONObj t = e.embeddedObject();
                 if ( t.firstElement().getGtLtOp() == 0 ) {
@@ -331,7 +337,8 @@ namespace mongo {
 
         ModSet( const BSONObj& from,
                 const set<string>& idxKeys = set<string>(),
-                const set<string>* backgroundKeys = 0 );
+                const set<string>* backgroundKeys = 0,
+                bool forReplication = false );
 
         /**
          * re-check if this mod is impacted by indexes
diff --git a/src/mongo/dbtests/repltests.cpp b/src/mongo/dbtests/repltests.cpp
index 1bea58636a5..505fc1c0bdb 100644
--- a/src/mongo/dbtests/repltests.cpp
+++ b/src/mongo/dbtests/repltests.cpp
@@ -1114,6 +1114,34 @@ namespace ReplTests {
             }
         };
 
+        //
+        // replay cases
+        //
+
+        class ReplaySetPreexistingNoOpPull : public Base {
+        public:
+            void doIt() const {
+                client()->update( ns(), BSONObj(), fromjson( "{$unset:{z:1}}" ));
+
+                // This is logged as {$set:{'a.b':[]},$set:{z:1}}, which might not be
+                // replayable against future versions of a document (here {_id:0,a:1,z:1}) due
+                // to SERVER-4781. As a result the $set:{z:1} will not be replayed in such
+                // cases (and also an exception may abort replication). If this were instead
+                // logged as {$set:{z:1}}, SERVER-4781 would not be triggered.
+                client()->update( ns(), BSONObj(), fromjson( "{$pull:{'a.b':1}, $set:{z:1}}" ) );
+                client()->update( ns(), BSONObj(), fromjson( "{$set:{a:1}}" ) );
+            }
+            using ReplTests::Base::check;
+            void check() const {
+                ASSERT_EQUALS( 1, count() );
+                check( fromjson( "{_id:0,a:1,z:1}" ), one( fromjson("{'_id':0}") ) );
+            }
+            void reset() const {
+                deleteAll( ns() );
+                insert( fromjson( "{'_id':0,a:{b:[]},z:1}" ) );
+            }
+        };
+
     } // namespace Idempotence
 
     class DeleteOpIsIdBased : public Base {
@@ -1534,6 +1562,7 @@ namespace ReplTests {
             add< Idempotence::IndexedSingletonNoRename >();
             add< Idempotence::AddToSetEmptyMissing >();
             add< Idempotence::AddToSetWithDollarSigns >();
+            add< Idempotence::ReplaySetPreexistingNoOpPull >();
             add< DeleteOpIsIdBased >();
             add< DatabaseIgnorerBasic >();
             add< DatabaseIgnorerUpdate >();
diff --git a/src/mongo/dbtests/updatetests.cpp b/src/mongo/dbtests/updatetests.cpp
index ac417879cb2..908ede74e37 100644
--- a/src/mongo/dbtests/updatetests.cpp
+++ b/src/mongo/dbtests/updatetests.cpp
@@ -679,8 +679,6 @@ namespace UpdateTests {
                 modSetState->applyModsInPlace(false);
                 ASSERT_EQUALS( BSON( "$set" << BSON( "a.0" << 3 ) ),
                                modSetState->getOpLogRewrite() );
-                // XXX we want inc to do a full $set to start with, not a positional one.
-                // XXX fix me.
             }
         };