SERVER-14668 Remove commitIfNeeded from the public RecoveryUnit API

Move its functionality to be under MMAP V1 code only.

13 files changed, 14 insertions, 71 deletions

diff --git a/src/mongo/db/catalog/database.cpp b/src/mongo/db/catalog/database.cpp
index c9356a39258..75f777e2877 100644
--- a/src/mongo/db/catalog/database.cpp
+++ b/src/mongo/db/catalog/database.cpp
@@ -95,11 +95,6 @@ namespace mongo {
         if ( BackgroundOperation::inProgForDb( _name ) ) {
             log() << "warning: bg op in prog during close db? " << _name << endl;
         }
-
-        // Before the files are closed, flush any potentially outstanding changes, which might
-        // reference this database. Otherwise we will assert when subsequent commit if needed
-        // is called and it happens to have write intents for the removed files.
-        txn->recoveryUnit()->commitIfNeeded(true);
     }
 
     Status Database::validateDBName( const StringData& dbname ) {
diff --git a/src/mongo/db/catalog/database_holder.cpp b/src/mongo/db/catalog/database_holder.cpp
index 1b5d20da12d..df9a92412ef 100644
--- a/src/mongo/db/catalog/database_holder.cpp
+++ b/src/mongo/db/catalog/database_holder.cpp
@@ -159,8 +159,6 @@ namespace mongo {
                                   bool force) {
         invariant(txn->lockState()->isW());
 
-        getDur().commitNow(txn); // bad things happen if we close a DB with outstanding writes
-
         SimpleMutex::scoped_lock lk(_m);
 
         set< string > dbs;
diff --git a/src/mongo/db/storage/README.md b/src/mongo/db/storage/README.md
index 69e42b12343..6d415f4c2dd 100644
--- a/src/mongo/db/storage/README.md
+++ b/src/mongo/db/storage/README.md
@@ -137,9 +137,8 @@ acts on that information.
 
 RecoveryUnit
 ------------
-**Q**: Should RecoveryUnit::syncDataAndTruncateJournal, RecoveryUnit::commitIfNeeded
-and RecoveryUnit::isCommitNeeded be static or in a different class? I am not
-sure why they need to effect the instance of RecoveryUnit?  
+**Q**: Should RecoveryUnit::syncDataAndTruncateJournal be static or in a different class? I am not
+sure why they need to effect the instance of RecoveryUnit?
 **A**: These methods are effectively static in that they will only modify global
 state. However, these methods are virtual, which is why we aren’t declaring them
 as static. That being said, we will likely remove this methods from the public
@@ -152,17 +151,6 @@ take a long time, is this expected to block until that is done?
 the user explicitly requests a sync via the fsync command.  We may rename this
 method as part of a naming sweep. 
 
-**Q**: I didn't see where RecoveryUnit::isCommitNeeded() is called and couldn't
-figure out how it is supposed to be used. What other handling we can possibly do
-other than issuing RecoveryUnit::commitIfNeeded()?  
-**A**: This will soon be removed from the API.
-
-**Q**: RecoveryUnit::commitIfNeeded, RecoveryUnit::isCommitNeeded - I assume this
-could be used to implement the InnoDB feature to force the log once per second  
-**A**: It’s used internally by the record store in mmapv1. We’ll soon make it
-private to DurRecoveryUnit and then remove it from the API. Ditto for various
-writingPtr methods.
-
 **Q**: As documented I don’t understand the point of the RecoverUnit::endUnitOfWork
 nesting behavior. Can you explain where it is used or will be used?  
 **A**: The RecoveryUnit interface and the mmapv1 (current mongodb storage engine)
@@ -181,22 +169,12 @@ and probably retry the operation. The interfaces are rather fluid right now and
 will probably return a Status (or throw an exception) at some point. However,
 rollback should never fail.
 
-**Q**: RecoveryUnit::commitIfNeeded() has a return value, but I didn't find codes
-where return value is used. What does false suppose to mean?  Does it mean I/O
-failure, wait to succeed, or wait to retry?  In general, in my understanding,
-RecoveryUnit::commitIfNeeded() is supposed to write out some partial data to
-transactional logs. If that's the case, maybe in
-RocksRecoveryUnit::commitIfNeeded(), we should call the write option that
-doesn't force fsync when writing to WAL, while in the final commit, force the
-WAL sync.  
-**A**: We’ll be removing commitIfNeeded from the public API.  It’s used by mmapv1
-internally.
 
 RocksDB
 -------
 **Q**: I think RocksRecoveryUnit::awaitCommit should remain in RecoveryUnit but be
 renamed to ::awaitLogSync. If force log once per second is done, then this
-blocks until the next commitIfNeededCall. But I think we [should] be explicit
+blocks until the next time log is forced to disk. But I think we [should] be explicit
 about "commit" vs "forcing redo log to storage" given that many engines
 including InnoDB, RocksDB & MongoDB let commit get done without a log force.  
 **A**: We agree that these should be two separately defined pieces of functionality.
diff --git a/src/mongo/db/storage/heap1/heap1_recovery_unit.h b/src/mongo/db/storage/heap1/heap1_recovery_unit.h
index 5f3c28d002b..3adf6d2be2e 100644
--- a/src/mongo/db/storage/heap1/heap1_recovery_unit.h
+++ b/src/mongo/db/storage/heap1/heap1_recovery_unit.h
@@ -45,10 +45,6 @@ namespace mongo {
 
         virtual void endUnitOfWork() {}
 
-        virtual bool commitIfNeeded(bool force = false) {
-            return false;
-        }
-
         virtual bool awaitCommit() {
             return true;
         }
diff --git a/src/mongo/db/storage/mmap_v1/dur.cpp b/src/mongo/db/storage/mmap_v1/dur.cpp
index 189383b663b..7d169e59713 100644
--- a/src/mongo/db/storage/mmap_v1/dur.cpp
+++ b/src/mongo/db/storage/mmap_v1/dur.cpp
@@ -197,7 +197,7 @@ namespace mongo {
             return false;
         }
 
-        bool NonDurableImpl::commitIfNeeded(OperationContext* txn, bool force) {
+        bool NonDurableImpl::commitIfNeeded(OperationContext* txn) {
             cc().checkpointHappened();   // XXX: remove when all dur goes through DurRecoveryUnit
             return false;
         }
@@ -328,12 +328,12 @@ namespace mongo {
 
             perf note: this function is called a lot, on every lock_w() ... and usually returns right away
         */
-        bool DurableImpl::commitIfNeeded(OperationContext* txn, bool force) {
+        bool DurableImpl::commitIfNeeded(OperationContext* txn) {
             // this is safe since since conceptually if you call commitIfNeeded, we're at a valid
             // spot in an operation to be terminated.
             cc().checkpointHappened();
 
-            if( likely( commitJob.bytes() < UncommittedBytesLimit && !force ) ) {
+            if (likely(commitJob.bytes() < UncommittedBytesLimit)) {
                 return false;
             }
             return _aCommitIsNeeded(txn);
diff --git a/src/mongo/db/storage/mmap_v1/dur.h b/src/mongo/db/storage/mmap_v1/dur.h
index d5df19b484c..53c46039957 100644
--- a/src/mongo/db/storage/mmap_v1/dur.h
+++ b/src/mongo/db/storage/mmap_v1/dur.h
@@ -131,7 +131,7 @@ namespace mongo {
                 from growing too large.
                 @return true if commited
             */
-            virtual bool commitIfNeeded(OperationContext* txn, bool force=false) = 0;
+            virtual bool commitIfNeeded(OperationContext* txn) = 0;
 
             /** Declare write intent for an int */
             inline int& writingInt(int& d) { return *static_cast<int*>(writingPtr( &d, sizeof(d))); }
@@ -187,7 +187,7 @@ namespace mongo {
             void createdFile(const std::string& filename, unsigned long long len) { }
             bool awaitCommit() { return false; }
             bool commitNow(OperationContext* txn);
-            bool commitIfNeeded(OperationContext* txn, bool force);
+            bool commitIfNeeded(OperationContext* txn);
             void syncDataAndTruncateJournal(OperationContext* txn) {}
             bool isDurable() const { return false; }
         };
@@ -201,7 +201,7 @@ namespace mongo {
             void createdFile(const std::string& filename, unsigned long long len);
             bool awaitCommit();
             bool commitNow(OperationContext* txn);
-            bool commitIfNeeded(OperationContext* txn, bool force);
+            bool commitIfNeeded(OperationContext* txn);
             void syncDataAndTruncateJournal(OperationContext* txn);
             bool isDurable() const { return true; }
         };
diff --git a/src/mongo/db/storage/mmap_v1/dur_recovery_unit.cpp b/src/mongo/db/storage/mmap_v1/dur_recovery_unit.cpp
index 0adf2c768dc..83209a7fa9b 100644
--- a/src/mongo/db/storage/mmap_v1/dur_recovery_unit.cpp
+++ b/src/mongo/db/storage/mmap_v1/dur_recovery_unit.cpp
@@ -159,15 +159,6 @@ namespace mongo {
         return getDur().awaitCommit();
     }
 
-    bool DurRecoveryUnit::commitIfNeeded(bool force) {
-        // TODO this method will be going away completely soon. There is only one remaining caller.
-        invariant(force);
-#if ROLLBACK_ENABLED
-        publishChanges();
-#endif
-        return getDur().commitIfNeeded(_txn, force);
-    }
-
     void* DurRecoveryUnit::writingPtr(void* data, size_t len) {
         invariant(len > 0);
 #if ROLLBACK_ENABLED
diff --git a/src/mongo/db/storage/mmap_v1/dur_recovery_unit.h b/src/mongo/db/storage/mmap_v1/dur_recovery_unit.h
index d23c3c27d36..df1f387eedc 100644
--- a/src/mongo/db/storage/mmap_v1/dur_recovery_unit.h
+++ b/src/mongo/db/storage/mmap_v1/dur_recovery_unit.h
@@ -53,8 +53,6 @@ namespace mongo {
 
         virtual bool awaitCommit();
 
-        virtual bool commitIfNeeded(bool force = false);
-
         virtual void* writingPtr(void* data, size_t len);
 
         //  The recovery unit takes ownership of change.
diff --git a/src/mongo/db/storage/mmap_v1/mmap_v1_engine.cpp b/src/mongo/db/storage/mmap_v1/mmap_v1_engine.cpp
index b36e8cc6953..90413952ce3 100644
--- a/src/mongo/db/storage/mmap_v1/mmap_v1_engine.cpp
+++ b/src/mongo/db/storage/mmap_v1/mmap_v1_engine.cpp
@@ -332,6 +332,11 @@ namespace {
     }
 
     Status MMAPV1Engine::closeDatabase( OperationContext* txn, const StringData& db ) {
+        // Before the files are closed, flush any potentially outstanding changes, which might
+        // reference this database. Otherwise we will assert when subsequent applications of the
+        // global journal entries occur, which happen to have write intents for the removed files.
+        getDur().commitNow(txn);
+
         boost::mutex::scoped_lock lk( _entryMapMutex );
         MMAPV1DatabaseCatalogEntry* entry = _entryMap[db.toString()];
         delete entry;
diff --git a/src/mongo/db/storage/recovery_unit.h b/src/mongo/db/storage/recovery_unit.h
index 69e073fbde3..4657a85c7f6 100644
--- a/src/mongo/db/storage/recovery_unit.h
+++ b/src/mongo/db/storage/recovery_unit.h
@@ -80,13 +80,6 @@ namespace mongo {
         virtual bool awaitCommit() = 0;
 
         /**
-         * Commit if required.  May take a long time.  Returns true if committed.
-         *
-         * WARNING: Data *must* be in a crash-recoverable state when this is called.
-         */
-        virtual bool commitIfNeeded(bool force = false) = 0;
-
-        /**
          * A Change is an action that is registerChange()'d while a WriteUnitOfWork exists. The
          * change is either rollback()'d or commit()'d when the WriteUnitOfWork goes out of scope.
          *
diff --git a/src/mongo/db/storage/recovery_unit_noop.h b/src/mongo/db/storage/recovery_unit_noop.h
index f50b7b41992..64a05e98d49 100644
--- a/src/mongo/db/storage/recovery_unit_noop.h
+++ b/src/mongo/db/storage/recovery_unit_noop.h
@@ -37,10 +37,6 @@ namespace mongo {
         virtual void commitUnitOfWork() {}
         virtual void endUnitOfWork() {}
 
-        virtual bool commitIfNeeded(bool force = false) {
-            return false;
-        }
-
         virtual bool awaitCommit() {
             return true;
         }
diff --git a/src/mongo/db/storage/rocks/rocks_recovery_unit.cpp b/src/mongo/db/storage/rocks/rocks_recovery_unit.cpp
index 5dbfd98bb09..4b296f60987 100644
--- a/src/mongo/db/storage/rocks/rocks_recovery_unit.cpp
+++ b/src/mongo/db/storage/rocks/rocks_recovery_unit.cpp
@@ -88,11 +88,6 @@ namespace mongo {
             commitUnitOfWork();
     }
 
-    bool RocksRecoveryUnit::commitIfNeeded(bool force ) {
-        commitUnitOfWork();
-        return true;
-    }
-
     bool RocksRecoveryUnit::awaitCommit() {
         // TODO
         return true;
diff --git a/src/mongo/db/storage/rocks/rocks_recovery_unit.h b/src/mongo/db/storage/rocks/rocks_recovery_unit.h
index 94f718c70b8..f49a9603d3d 100644
--- a/src/mongo/db/storage/rocks/rocks_recovery_unit.h
+++ b/src/mongo/db/storage/rocks/rocks_recovery_unit.h
@@ -59,8 +59,6 @@ namespace mongo {
 
         virtual void endUnitOfWork();
 
-        virtual bool commitIfNeeded(bool force = false);
-
         virtual bool awaitCommit();
 
         virtual void* writingPtr(void* data, size_t len);