SERVER-37867 Cleanup the RecordStore insertRecord(s)/dataFor API

13 files changed, 117 insertions, 150 deletions

diff --git a/src/mongo/db/storage/biggie/biggie_record_store.cpp b/src/mongo/db/storage/biggie/biggie_record_store.cpp
index 02a7f245bcd..e65158a3e09 100644
--- a/src/mongo/db/storage/biggie/biggie_record_store.cpp
+++ b/src/mongo/db/storage/biggie/biggie_record_store.cpp
@@ -136,12 +136,6 @@ int64_t RecordStore::storageSize(OperationContext* opCtx,
     return dataSize(opCtx);
 }
 
-RecordData RecordStore::dataFor(OperationContext* opCtx, const RecordId& loc) const {
-    RecordData rd;
-    invariant(findRecord(opCtx, loc, &rd));
-    return rd;
-}
-
 bool RecordStore::findRecord(OperationContext* opCtx, const RecordId& loc, RecordData* rd) const {
     const StringStore* workingCopy = getRecoveryUnitBranch_forking(opCtx);
     auto it = workingCopy->find(createKey(_ident, loc.repr()));
@@ -159,16 +153,18 @@ void RecordStore::deleteRecord(OperationContext* opCtx, const RecordId& dl) {
     dirtyRecoveryUnit(opCtx);
 }
 
-StatusWith<RecordId> RecordStore::insertRecord(OperationContext* opCtx,
-                                               const char* data,
-                                               int len,
-                                               Timestamp) {
-    int64_t thisRecordId = nextRecordId();
-    StringStore* workingCopy = getRecoveryUnitBranch_forking(opCtx);
-    workingCopy->insert(
-        StringStore::value_type{createKey(_ident, thisRecordId), std::string(data, len)});
-    dirtyRecoveryUnit(opCtx);
-    return StatusWith<RecordId>(RecordId(thisRecordId));
+Status RecordStore::insertRecords(OperationContext* opCtx,
+                                  std::vector<Record>* inOutRecords,
+                                  const std::vector<Timestamp>& timestamps) {
+    for (auto& record : *inOutRecords) {
+        int64_t thisRecordId = nextRecordId();
+        StringStore* workingCopy = getRecoveryUnitBranch_forking(opCtx);
+        workingCopy->insert(StringStore::value_type{
+            createKey(_ident, thisRecordId), std::string(record.data.data(), record.data.size())});
+        record.id = RecordId(thisRecordId);
+        dirtyRecoveryUnit(opCtx);
+    }
+    return Status::OK();
 }
 
 Status RecordStore::insertRecordsWithDocWriter(OperationContext* opCtx,
diff --git a/src/mongo/db/storage/biggie/biggie_record_store.h b/src/mongo/db/storage/biggie/biggie_record_store.h
index cd13b51452d..5d11ddaa98b 100644
--- a/src/mongo/db/storage/biggie/biggie_record_store.h
+++ b/src/mongo/db/storage/biggie/biggie_record_store.h
@@ -75,17 +75,13 @@ public:
                                 BSONObjBuilder* extraInfo = NULL,
                                 int infoLevel = 0) const;
 
-    virtual RecordData dataFor(OperationContext* opCtx, const RecordId& loc) const;
-
     virtual bool findRecord(OperationContext* opCtx, const RecordId& loc, RecordData* rd) const;
 
     virtual void deleteRecord(OperationContext* opCtx, const RecordId& dl);
 
-    virtual StatusWith<RecordId> insertRecord(OperationContext* opCtx,
-                                              const char* data,
-                                              int len,
-                                              Timestamp);
-
+    virtual Status insertRecords(OperationContext* opCtx,
+                                 std::vector<Record>* inOutRecords,
+                                 const std::vector<Timestamp>& timestamps);
 
     virtual Status insertRecordsWithDocWriter(OperationContext* opCtx,
                                               const DocWriter* const* docs,
diff --git a/src/mongo/db/storage/devnull/devnull_kv_engine.cpp b/src/mongo/db/storage/devnull/devnull_kv_engine.cpp
index a7d76ddf7ad..4a37c3b751b 100644
--- a/src/mongo/db/storage/devnull/devnull_kv_engine.cpp
+++ b/src/mongo/db/storage/devnull/devnull_kv_engine.cpp
@@ -93,22 +93,20 @@ public:
         return 0;
     }
 
-    virtual RecordData dataFor(OperationContext* opCtx, const RecordId& loc) const {
-        return RecordData(_dummy.objdata(), _dummy.objsize());
-    }
-
     virtual bool findRecord(OperationContext* opCtx, const RecordId& loc, RecordData* rd) const {
         return false;
     }
 
     virtual void deleteRecord(OperationContext* opCtx, const RecordId& dl) {}
 
-    virtual StatusWith<RecordId> insertRecord(OperationContext* opCtx,
-                                              const char* data,
-                                              int len,
-                                              Timestamp) {
-        _numInserts++;
-        return StatusWith<RecordId>(RecordId(6, 4));
+    virtual Status insertRecords(OperationContext* opCtx,
+                                 std::vector<Record>* inOutRecords,
+                                 const std::vector<Timestamp>& timestamps) {
+        _numInserts += inOutRecords->size();
+        for (auto& record : *inOutRecords) {
+            record.id = RecordId(6, 4);
+        }
+        return Status::OK();
     }
 
     virtual Status insertRecordsWithDocWriter(OperationContext* opCtx,
diff --git a/src/mongo/db/storage/ephemeral_for_test/ephemeral_for_test_record_store.cpp b/src/mongo/db/storage/ephemeral_for_test/ephemeral_for_test_record_store.cpp
index 27df7fceb71..bf5ed977822 100644
--- a/src/mongo/db/storage/ephemeral_for_test/ephemeral_for_test_record_store.cpp
+++ b/src/mongo/db/storage/ephemeral_for_test/ephemeral_for_test_record_store.cpp
@@ -398,37 +398,50 @@ StatusWith<RecordId> EphemeralForTestRecordStore::extractAndCheckLocForOplog(con
     return status;
 }
 
-StatusWith<RecordId> EphemeralForTestRecordStore::insertRecord(OperationContext* opCtx,
-                                                               const char* data,
-                                                               int len,
-                                                               Timestamp) {
-    if (_isCapped && len > _cappedMaxSize) {
-        // We use dataSize for capped rollover and we don't want to delete everything if we know
-        // this won't fit.
-        return StatusWith<RecordId>(ErrorCodes::BadValue, "object to insert exceeds cappedMaxSize");
+Status EphemeralForTestRecordStore::insertRecords(OperationContext* opCtx,
+                                                  std::vector<Record>* inOutRecords,
+                                                  const std::vector<Timestamp>& timestamps) {
+
+    for (auto& record : *inOutRecords) {
+        if (_isCapped && record.data.size() > _cappedMaxSize) {
+            // We use dataSize for capped rollover and we don't want to delete everything if we know
+            // this won't fit.
+            return Status(ErrorCodes::BadValue, "object to insert exceeds cappedMaxSize");
+        }
     }
+    const auto insertSingleFn = [this, opCtx](Record* record) {
+        stdx::lock_guard<stdx::recursive_mutex> lock(_data->recordsMutex);
+        EphemeralForTestRecord rec(record->data.size());
+        memcpy(rec.data.get(), record->data.data(), record->data.size());
 
-    stdx::lock_guard<stdx::recursive_mutex> lock(_data->recordsMutex);
-    EphemeralForTestRecord rec(len);
-    memcpy(rec.data.get(), data, len);
+        RecordId loc;
+        if (_data->isOplog) {
+            StatusWith<RecordId> status =
+                extractAndCheckLocForOplog(record->data.data(), record->data.size());
+            if (!status.isOK())
+                return status.getStatus();
+            loc = status.getValue();
+        } else {
+            loc = allocateLoc();
+        }
 
-    RecordId loc;
-    if (_data->isOplog) {
-        StatusWith<RecordId> status = extractAndCheckLocForOplog(data, len);
+        _data->dataSize += record->data.size();
+        _data->records[loc] = rec;
+        record->id = loc;
+
+        opCtx->recoveryUnit()->registerChange(new InsertChange(opCtx, _data, loc));
+        cappedDeleteAsNeeded_inlock(opCtx);
+
+        return Status::OK();
+    };
+
+    for (auto& record : *inOutRecords) {
+        auto status = insertSingleFn(&record);
         if (!status.isOK())
             return status;
-        loc = status.getValue();
-    } else {
-        loc = allocateLoc();
     }
 
-    opCtx->recoveryUnit()->registerChange(new InsertChange(opCtx, _data, loc));
-    _data->dataSize += len;
-    _data->records[loc] = rec;
-
-    cappedDeleteAsNeeded_inlock(opCtx);
-
-    return StatusWith<RecordId>(loc);
+    return Status::OK();
 }
 
 Status EphemeralForTestRecordStore::insertRecordsWithDocWriter(OperationContext* opCtx,
diff --git a/src/mongo/db/storage/ephemeral_for_test/ephemeral_for_test_record_store.h b/src/mongo/db/storage/ephemeral_for_test/ephemeral_for_test_record_store.h
index 03ca2e733ca..419d68e25c3 100644
--- a/src/mongo/db/storage/ephemeral_for_test/ephemeral_for_test_record_store.h
+++ b/src/mongo/db/storage/ephemeral_for_test/ephemeral_for_test_record_store.h
@@ -68,10 +68,9 @@ public:
 
     virtual void deleteRecord(OperationContext* opCtx, const RecordId& dl);
 
-    virtual StatusWith<RecordId> insertRecord(OperationContext* opCtx,
-                                              const char* data,
-                                              int len,
-                                              Timestamp);
+    virtual Status insertRecords(OperationContext* opCtx,
+                                 std::vector<Record>* inOutRecords,
+                                 const std::vector<Timestamp>& timestamps);
 
     virtual Status insertRecordsWithDocWriter(OperationContext* opCtx,
                                               const DocWriter* const* docs,
diff --git a/src/mongo/db/storage/kv/SConscript b/src/mongo/db/storage/kv/SConscript
index ea5b289b1e7..ebd02a8a6f7 100644
--- a/src/mongo/db/storage/kv/SConscript
+++ b/src/mongo/db/storage/kv/SConscript
@@ -93,7 +93,7 @@ env.Library(
     source=[
         'kv_catalog_feature_tracker_test.cpp',
         'kv_engine_test_harness.cpp',
-        'kv_engine_test_timestamps.cpp',
+        'kv_engine_timestamps_test.cpp',
         ],
     LIBDEPS=[
         '$BUILD_DIR/mongo/db/service_context',
diff --git a/src/mongo/db/storage/kv/kv_database_catalog_entry_base.h b/src/mongo/db/storage/kv/kv_database_catalog_entry_base.h
index cdce94f2209..2a14e8e0e84 100644
--- a/src/mongo/db/storage/kv/kv_database_catalog_entry_base.h
+++ b/src/mongo/db/storage/kv/kv_database_catalog_entry_base.h
@@ -84,7 +84,6 @@ public:
 
     void initCollection(OperationContext* opCtx, const std::string& ns, bool forRepair);
 
-    void initCollectionBeforeRepair(OperationContext* opCtx, const std::string& ns);
     void reinitCollectionAfterRepair(OperationContext* opCtx, const std::string& ns);
 
 protected:
diff --git a/src/mongo/db/storage/kv/kv_engine_test_timestamps.cpp b/src/mongo/db/storage/kv/kv_engine_timestamps_test.cpp
index fb935460769..fb935460769 100644
--- a/src/mongo/db/storage/kv/kv_engine_test_timestamps.cpp
+++ b/src/mongo/db/storage/kv/kv_engine_timestamps_test.cpp
diff --git a/src/mongo/db/storage/mobile/mobile_record_store.cpp b/src/mongo/db/storage/mobile/mobile_record_store.cpp
index e588ebe9e36..d6e2c4688ec 100644
--- a/src/mongo/db/storage/mobile/mobile_record_store.cpp
+++ b/src/mongo/db/storage/mobile/mobile_record_store.cpp
@@ -259,13 +259,6 @@ long long MobileRecordStore::numRecords(OperationContext* opCtx) const {
     return _numRecs;
 }
 
-RecordData MobileRecordStore::dataFor(OperationContext* opCtx, const RecordId& recId) const {
-    RecordData recData;
-    bool recFound = findRecord(opCtx, recId, &recData);
-    invariant(recFound);
-    return recData;
-}
-
 bool MobileRecordStore::findRecord(OperationContext* opCtx,
                                    const RecordId& recId,
                                    RecordData* rd) const {
@@ -305,25 +298,31 @@ void MobileRecordStore::deleteRecord(OperationContext* opCtx, const RecordId& re
     deleteStmt.step(SQLITE_DONE);
 }
 
-StatusWith<RecordId> MobileRecordStore::insertRecord(OperationContext* opCtx,
-                                                     const char* data,
-                                                     int len,
-                                                     Timestamp) {
+Status MobileRecordStore::insertRecords(OperationContext* opCtx,
+                                        std::vector<Record>* inOutRecords,
+                                        const std::vector<Timestamp>& timestamps) {
     // Inserts record into SQLite table (or replaces if duplicate record id).
     MobileSession* session = MobileRecoveryUnit::get(opCtx)->getSession(opCtx, false);
 
-    _changeNumRecs(opCtx, 1);
-    _changeDataSize(opCtx, len);
+    for (auto& record : *inOutRecords) {
+        const auto data = record.data.data();
+        const auto len = record.data.size();
+
+        _changeNumRecs(opCtx, 1);
+        _changeDataSize(opCtx, len);
 
-    std::string insertQuery =
-        "INSERT OR REPLACE INTO \"" + _ident + "\"(rec_id, data) VALUES(?, ?);";
-    SqliteStatement insertStmt(*session, insertQuery);
-    RecordId recId = _nextId();
-    insertStmt.bindInt(0, recId.repr());
-    insertStmt.bindBlob(1, data, len);
-    insertStmt.step(SQLITE_DONE);
+        std::string insertQuery =
+            "INSERT OR REPLACE INTO \"" + _ident + "\"(rec_id, data) VALUES(?, ?);";
+        SqliteStatement insertStmt(*session, insertQuery);
+        RecordId recId = _nextId();
+        insertStmt.bindInt(0, recId.repr());
+        insertStmt.bindBlob(1, data, len);
+        insertStmt.step(SQLITE_DONE);
 
-    return StatusWith<RecordId>(recId);
+        record.id = recId;
+    }
+
+    return Status::OK();
 }
 
 Status MobileRecordStore::insertRecordsWithDocWriter(OperationContext* opCtx,
diff --git a/src/mongo/db/storage/mobile/mobile_record_store.h b/src/mongo/db/storage/mobile/mobile_record_store.h
index fb9eb2fcd13..0c31e35cd13 100644
--- a/src/mongo/db/storage/mobile/mobile_record_store.h
+++ b/src/mongo/db/storage/mobile/mobile_record_store.h
@@ -56,16 +56,13 @@ public:
 
     const std::string& getIdent() const override;
 
-    RecordData dataFor(OperationContext* opCtx, const RecordId& recId) const override;
-
     bool findRecord(OperationContext* opCtx, const RecordId& recId, RecordData* rd) const override;
 
     void deleteRecord(OperationContext* opCtx, const RecordId& dl) override;
 
-    StatusWith<RecordId> insertRecord(OperationContext* opCtx,
-                                      const char* data,
-                                      int len,
-                                      Timestamp timestamp) override;
+    Status insertRecords(OperationContext* opCtx,
+                         std::vector<Record>* inOutRecords,
+                         const std::vector<Timestamp>& timestamps) override;
 
     Status insertRecordsWithDocWriter(OperationContext* opCtx,
                                       const DocWriter* const* docs,
diff --git a/src/mongo/db/storage/record_store.h b/src/mongo/db/storage/record_store.h
index 36c09860fbf..ecee15a07c7 100644
--- a/src/mongo/db/storage/record_store.h
+++ b/src/mongo/db/storage/record_store.h
@@ -1,5 +1,3 @@
-// record_store.h
-
 
 /**
  *    Copyright (C) 2018-present MongoDB, Inc.
@@ -261,7 +259,7 @@ public:
     // name of the RecordStore implementation
     virtual const char* name() const = 0;
 
-    virtual const std::string& ns() const {
+    const std::string& ns() const {
         return _ns;
     }
 
@@ -303,13 +301,15 @@ public:
     /**
      * Get the RecordData at loc, which must exist.
      *
-     * If unowned data is returned, it is valid until the next modification of this Record or
-     * the lock on this collection is released.
+     * If unowned data is returned, it is only valid until either of these happens:
+     *  - The record is modified
+     *  - The snapshot from which it was obtained is abandoned
+     *  - The lock on the collection is released
      *
-     * In general, prefer findRecord or RecordCursor::seekExact since they can tell you if a
-     * record has been removed.
+     * In general, prefer findRecord or RecordCursor::seekExact since they can tell you if a record
+     * has been removed.
      */
-    virtual RecordData dataFor(OperationContext* opCtx, const RecordId& loc) const {
+    RecordData dataFor(OperationContext* opCtx, const RecordId& loc) const {
         RecordData data;
         invariant(findRecord(opCtx, loc, &data));
         return data;
@@ -342,24 +342,26 @@ public:
 
     virtual void deleteRecord(OperationContext* opCtx, const RecordId& dl) = 0;
 
-    virtual StatusWith<RecordId> insertRecord(OperationContext* opCtx,
-                                              const char* data,
-                                              int len,
-                                              Timestamp timestamp) = 0;
-
+    /**
+     * Inserts the specified records into this RecordStore by copying the passed-in record data and
+     * updates 'inOutRecords' to contain the ids of the inserted records.
+     */
     virtual Status insertRecords(OperationContext* opCtx,
-                                 std::vector<Record>* records,
-                                 std::vector<Timestamp>* timestamps) {
-        int index = 0;
-        for (auto& record : *records) {
-            StatusWith<RecordId> res =
-                insertRecord(opCtx, record.data.data(), record.data.size(), (*timestamps)[index++]);
-            if (!res.isOK())
-                return res.getStatus();
-
-            record.id = res.getValue();
-        }
-        return Status::OK();
+                                 std::vector<Record>* inOutRecords,
+                                 const std::vector<Timestamp>& timestamps) = 0;
+
+    /**
+     * A thin wrapper around insertRecords() to simplify handling of single document inserts.
+     */
+    StatusWith<RecordId> insertRecord(OperationContext* opCtx,
+                                      const char* data,
+                                      int len,
+                                      Timestamp timestamp) {
+        std::vector<Record> inOutRecords{Record{RecordId(), RecordData(data, len)}};
+        Status status = insertRecords(opCtx, &inOutRecords, std::vector<Timestamp>{timestamp});
+        if (!status.isOK())
+            return status;
+        return inOutRecords.front().id;
     }
 
     /**
diff --git a/src/mongo/db/storage/wiredtiger/wiredtiger_record_store.cpp b/src/mongo/db/storage/wiredtiger/wiredtiger_record_store.cpp
index 48d3248e2dc..a8e8083802f 100644
--- a/src/mongo/db/storage/wiredtiger/wiredtiger_record_store.cpp
+++ b/src/mongo/db/storage/wiredtiger/wiredtiger_record_store.cpp
@@ -821,20 +821,6 @@ RecordData WiredTigerRecordStore::_getData(const WiredTigerCursor& cursor) const
     return RecordData(static_cast<const char*>(value.data), value.size).getOwned();
 }
 
-RecordData WiredTigerRecordStore::dataFor(OperationContext* opCtx, const RecordId& id) const {
-    dassert(opCtx->lockState()->isReadLocked());
-
-    // ownership passes to the shared_array created below
-    WiredTigerCursor curwrap(_uri, _tableId, true, opCtx);
-    WT_CURSOR* c = curwrap.get();
-    invariant(c);
-    setKey(c, id);
-    int ret = wiredTigerPrepareConflictRetry(opCtx, [&] { return c->search(c); });
-    massert(28556, "Didn't find RecordId in WiredTigerRecordStore", ret != WT_NOTFOUND);
-    invariantWTOK(ret);
-    return _getData(curwrap);
-}
-
 bool WiredTigerRecordStore::findRecord(OperationContext* opCtx,
                                        const RecordId& id,
                                        RecordData* out) const {
@@ -1229,8 +1215,8 @@ void WiredTigerRecordStore::reclaimOplog(OperationContext* opCtx, Timestamp mayT
 
 Status WiredTigerRecordStore::insertRecords(OperationContext* opCtx,
                                             std::vector<Record>* records,
-                                            std::vector<Timestamp>* timestamps) {
-    return _insertRecords(opCtx, records->data(), timestamps->data(), records->size());
+                                            const std::vector<Timestamp>& timestamps) {
+    return _insertRecords(opCtx, records->data(), timestamps.data(), records->size());
 }
 
 Status WiredTigerRecordStore::_insertRecords(OperationContext* opCtx,
@@ -1313,17 +1299,6 @@ Status WiredTigerRecordStore::_insertRecords(OperationContext* opCtx,
     return Status::OK();
 }
 
-StatusWith<RecordId> WiredTigerRecordStore::insertRecord(OperationContext* opCtx,
-                                                         const char* data,
-                                                         int len,
-                                                         Timestamp timestamp) {
-    Record record = {RecordId(), RecordData(data, len)};
-    Status status = _insertRecords(opCtx, &record, &timestamp, 1);
-    if (!status.isOK())
-        return StatusWith<RecordId>(status);
-    return StatusWith<RecordId>(record.id);
-}
-
 bool WiredTigerRecordStore::isOpHidden_forTest(const RecordId& id) const {
     invariant(id.repr() > 0);
     invariant(_kvEngine->getOplogManager()->isRunning());
diff --git a/src/mongo/db/storage/wiredtiger/wiredtiger_record_store.h b/src/mongo/db/storage/wiredtiger/wiredtiger_record_store.h
index 6fdfbd4f9d9..f450193fe0c 100644
--- a/src/mongo/db/storage/wiredtiger/wiredtiger_record_store.h
+++ b/src/mongo/db/storage/wiredtiger/wiredtiger_record_store.h
@@ -137,20 +137,13 @@ public:
 
     // CRUD related
 
-    virtual RecordData dataFor(OperationContext* opCtx, const RecordId& id) const;
-
     virtual bool findRecord(OperationContext* opCtx, const RecordId& id, RecordData* out) const;
 
     virtual void deleteRecord(OperationContext* opCtx, const RecordId& id);
 
     virtual Status insertRecords(OperationContext* opCtx,
                                  std::vector<Record>* records,
-                                 std::vector<Timestamp>* timestamps);
-
-    virtual StatusWith<RecordId> insertRecord(OperationContext* opCtx,
-                                              const char* data,
-                                              int len,
-                                              Timestamp timestamp);
+                                 const std::vector<Timestamp>& timestamps);
 
     virtual Status insertRecordsWithDocWriter(OperationContext* opCtx,
                                               const DocWriter* const* docs,