Skip to content

Commit

Permalink
MB-45917: Use correct "diskSize" for compaction after an upgrade
Browse files Browse the repository at this point in the history 
The collection diskSize does not exist in 6.5 couchstore files, so
the code uses dbinfo.space_used (as only the default collection
exists, this is a reasonable over-estimate).

During 7.0 compaction the collection diskSize stat must be updated
if tombstones are purged and MB-45917 identified a problem in
compaction where the collection stats are read from the new file,
which is generally fine except in the case where an upgrade just
occurred, because the disk-size is set to the space_used of the
compacted file.

If compaction has just purged many tombstones the space_used can be
small and the purged-byte counter large -> underflow.

Fix this by updating couchstore so that both source/target Db& are
passed to the precommit hook and then read the stats from the source.
Now we will subtract from a large size and not underflow the disk
size.

A second fix is included that was found in the added unit-test where
the on_disk_prepares was accessed but not present. This could be an
issue with the unit test hooks, but the code is now more robust if a
path is possible where we get to 7.0 without that field present in
vbstate.

Change-Id: I7b00f75738c024b91acbefb8b4eb7f739fa185cd
Reviewed-on: http://review.couchbase.org/c/kv_engine/+/152195
Tested-by: Build Bot <build@couchbase.com>
Reviewed-by: Paolo Cocchi <paolo.cocchi@couchbase.com>
  • Loading branch information
@jimwwalker
jimwwalker committed Apr 27, 2021
1 parent 3a5a7aa commit 0917f06
Show file tree
Hide file tree
Showing 3 changed files with 104 additions and 52 deletions.
48 changes: 28 additions & 20 deletions engines/ep/src/couch-kvstore/couch-kvstore.cc
View file
Expand Up @@ -1082,11 +1082,7 @@ couchstore_error_t CouchKVStore::replayPrecommitHook(
}

try {
if (std::stoul(json["on_disk_prepares"].get<std::string>()) !=
prepareStats.onDiskPrepares) {
json["on_disk_prepares"] =
std::to_string(prepareStats.onDiskPrepares);
}
json["on_disk_prepares"] = std::to_string(prepareStats.onDiskPrepares);

auto prepareBytes = json.find("on_disk_prepare_bytes");
// only update if it's there..
Expand Down Expand Up @@ -1377,12 +1373,15 @@ CouchKVStore::CompactDBInternalStatus CouchKVStore::compactDBInternal(
},
{},
def_iops,
[vbid, hook_ctx, this](Db& compacted) {
[vbid, hook_ctx, this](Db& source, Db& compacted) {
// we don't try to delete the dropped collection document
// as it'll come back in the next database header anyway
PendingLocalDocRequestQueue localDocQueue;
auto ret = maybePatchMetaData(
compacted, hook_ctx->stats, localDocQueue, vbid);
auto ret = maybePatchMetaData(source,
compacted,
hook_ctx->stats,
localDocQueue,
vbid);
if (ret == COUCHSTORE_SUCCESS) {
ret = updateLocalDocuments(compacted, localDocQueue);
}
Expand Down Expand Up @@ -1425,8 +1424,8 @@ CouchKVStore::CompactDBInternalStatus CouchKVStore::compactDBInternal(
target, docInfo, prepareStats, collectionStats);
},
[&prepareStats, &collectionStats, &purge_seqno, hook_ctx, this](
Db& db) {
return replayPrecommitHook(db,
Db&, Db& compacted) {
return replayPrecommitHook(compacted,
prepareStats,
collectionStats,
purge_seqno,
Expand Down Expand Up @@ -1466,7 +1465,7 @@ CouchKVStore::CompactDBInternalStatus CouchKVStore::compactDBInternal(
},
{},
def_iops,
[vbid, hook_ctx, this](Db& compacted) {
[vbid, hook_ctx, this](Db& source, Db& compacted) {
if (mb40415_regression_hook) {
return COUCHSTORE_ERROR_CANCEL;
}
Expand All @@ -1481,8 +1480,11 @@ CouchKVStore::CompactDBInternalStatus CouchKVStore::compactDBInternal(
hook_ctx->eraserContext->setOnDiskDroppedDataExists(
false);
}
auto ret = maybePatchMetaData(
compacted, hook_ctx->stats, localDocQueue, vbid);
auto ret = maybePatchMetaData(source,
compacted,
hook_ctx->stats,
localDocQueue,
vbid);
if (ret == COUCHSTORE_SUCCESS) {
ret = updateLocalDocuments(compacted, localDocQueue);
}
Expand Down Expand Up @@ -1530,8 +1532,11 @@ CouchKVStore::CompactDBInternalStatus CouchKVStore::compactDBInternal(
couchstore_strerror(status));
return CompactDBInternalStatus::Failed;
}

// This field may not be present if first compaction after upgrade from
// less than 6.5 to 7.0, e.g. 6.0 -> 7.0
prepareStats.onDiskPrepares =
std::stoul(json["on_disk_prepares"].get<std::string>());
std::stoul(json.value("on_disk_prepares", "0"));

auto prepareBytes = json.find("on_disk_prepare_bytes");
// only update if it's there..
Expand Down Expand Up @@ -1697,7 +1702,8 @@ bool CouchKVStore::compactDBTryAndSwitchToNewFile(
}

couchstore_error_t CouchKVStore::maybePatchMetaData(
Db& db,
Db& source,
Db& target,
CompactionStats& stats,
PendingLocalDocRequestQueue& localDocQueue,
Vbid vbid) {
Expand All @@ -1706,7 +1712,7 @@ couchstore_error_t CouchKVStore::maybePatchMetaData(
// Must sync the modified state back we need to update the
// _local/vbstate to update the number of on_disk_prepares to match
// whatever we purged
auto [status, json] = getLocalVbState(db);
auto [status, json] = getLocalVbState(target);
if (status != COUCHSTORE_SUCCESS) {
logger.warn(
"CouchKVStore::maybePatchMetaData: Failed to load "
Expand Down Expand Up @@ -1770,8 +1776,11 @@ couchstore_error_t CouchKVStore::maybePatchMetaData(

// Finally see if compaction purged data and disk size stats need updates
for (auto [cid, purgedBytes] : stats.collectionSizeUpdates) {
// Need to read the collection stats
auto [success, currentStats] = getCollectionStats(db, cid);
// Need to read the collection stats. We read from the source file so
// that in the case of a first compaction after upgrade from pre 7.0
// we get the correct disk (because we use the dbinfo.space_used). The
// target file could return a value too small in that case (MB-45917)
auto [success, currentStats] = getCollectionStats(source, cid);
if (!success) {
logger.warn(
"CouchKVStore::maybePatchMetaData: Failed to load "
Expand Down Expand Up @@ -1799,7 +1808,6 @@ couchstore_error_t CouchKVStore::maybePatchMetaData(
return COUCHSTORE_SUCCESS;
}


bool CouchKVStore::tryToCatchUpDbFile(Db& source,
Db& destination,
std::unique_lock<std::mutex>& lock,
Expand Down Expand Up @@ -1887,7 +1895,7 @@ bool CouchKVStore::tryToCatchUpDbFile(Db& source,
target, docInfo, prepareStats, collectionStats);
},
[&prepareStats, &collectionStats, &purge_seqno, &hook_ctx, this](
Db& compacted) {
Db&, Db& compacted) {
return replayPrecommitHook(compacted,
prepareStats,
collectionStats,
Expand Down
6 changes: 4 additions & 2 deletions engines/ep/src/couch-kvstore/couch-kvstore.h
View file
Expand Up @@ -772,13 +772,15 @@ class CouchKVStore : public KVStore
* The collection stats may need updating if documents in collections were
* purged.
*
* @param db The compacted database
* @param source The source of compaction
* @param target The compacted database
* @param stats Data tracked by compaction used for stat updates
* @param localDocQueue the queue which will be updated with new documents
* @param vbid vbucket being compacted
*/
couchstore_error_t maybePatchMetaData(
Db& db,
Db& source,
Db& target,
CompactionStats& stats,
PendingLocalDocRequestQueue& localDocQueue,
Vbid vbid);
Expand Down
102 changes: 72 additions & 30 deletions engines/ep/tests/module_tests/couch-kvstore_test.cc
View file
Expand Up @@ -42,7 +42,24 @@ class CouchKVStoreTest : public KVStoreTest {
CouchKVStoreTest() : KVStoreTest() {
}

void collectionsOfflineUpgrade(bool writeAsMadHatter);
/**
* Run an offline upgrade
* @param writeAsMadHatter Make the data file appear as if it's 6.5
* @param outputCid Upgrade and move the keys to this collection
* @param keys how many keys to write
* @param deleteKeyRange a range of keys that will be deleted
*/
void collectionsOfflineUpgrade(bool writeAsMadHatter,
CollectionID outputCid,
int keys,
std::pair<int, int> deletedKeyRange);

void runCompaction(KVStore& kvstore, CompactionConfig config) {
std::mutex mutex;
std::unique_lock<std::mutex> lock(mutex);
auto ctx = std::make_shared<CompactionContext>(Vbid(0), config, 0);
EXPECT_TRUE(kvstore.compactDB(lock, ctx));
}
};

// Verify the stats returned from operations are accurate.
Expand Down Expand Up @@ -173,18 +190,18 @@ class CollectionsOfflineUpgradeCallback : public StatusCallback<CacheLookup> {

class CollectionsOfflineGetCallback : public StatusCallback<GetValue> {
public:
explicit CollectionsOfflineGetCallback(std::pair<int, int> deletedRange)
: deletedRange(std::move(deletedRange)) {
explicit CollectionsOfflineGetCallback(CollectionID expectedId,
std::pair<int, int> deletedRange)
: expectedId(expectedId), deletedRange(std::move(deletedRange)) {
}

void callback(GetValue& result) override {
EXPECT_EQ(cb::engine_errc::success, result.getStatus());

if (result.item->isDeleted()) {
DocKey dk = result.item->getKey();
EXPECT_EQ(500, dk.getCollectionID());
EXPECT_EQ(expectedId, dk.getCollectionID());
auto noCollection = dk.makeDocKeyWithoutCollectionID();
EXPECT_EQ(3, noCollection.size());
// create a string from the logical-key, i.e. +1 and skip the
// collection-ID
std::string str(
Expand All @@ -206,33 +223,38 @@ class CollectionsOfflineGetCallback : public StatusCallback<GetValue> {
result.item->getDataType());
result.item->decompressValue();

EXPECT_EQ(0,
strncmp("valuable",
result.item->getData(),
result.item->getNBytes()));
std::string_view value{result.item->getData(),
result.item->getNBytes()};

std::string expectedValue = "valuable";
if (result.item->getDataType() & PROTOCOL_BINARY_DATATYPE_XATTR) {
expectedValue = createXattrValue("value");
}
EXPECT_EQ(expectedValue, value);
}

private:
CollectionID expectedId;
std::pair<int, int> deletedRange;
};

// Test the InputCouchFile/OutputCouchFile objects (in a simple test) to
// check they do what we expect, that is create a new couchfile with all keys
// moved into a specified collection.
void CouchKVStoreTest::collectionsOfflineUpgrade(bool writeAsMadHatter) {
void CouchKVStoreTest::collectionsOfflineUpgrade(
bool writeAsMadHatter,
CollectionID outputCid,
int keys,
std::pair<int, int> deletedRange) {
ASSERT_LE(deletedRange.first, deletedRange.second);
ASSERT_LE(deletedRange.second, keys);
CouchKVStoreConfig config1(1024, 4, data_dir, "couchdb", 0);

CouchKVStoreConfig config2(1024, 4, data_dir, "couchdb", 0);

// Test setup, create a new file
auto kvstore = setup_kv_store(config1);
kvstore->begin(std::make_unique<TransactionContext>(vbid));

// The key count is large enough to ensure the count uses more than 1 byte
// of leb storage so we validate that leb encode/decode works on this path
const int keys = 129;
const int deletedKeys = 14;

for (int i = 0; i < keys; i++) {
// key needs to look like it's in the default collection so we can flush
// it
Expand All @@ -248,25 +270,26 @@ void CouchKVStoreTest::collectionsOfflineUpgrade(bool writeAsMadHatter) {

kvstore->begin(std::make_unique<TransactionContext>(vbid));

// Delete some keys. With and without a value (like xattr)
for (int i = 18, j = 1; i < 18 + deletedKeys; ++i, ++j) {
// Now delete keys (if requested). Alternate between value and no-value
// (like xattr)
for (int i = deletedRange.first, seqno = keys; i < deletedRange.second;
++i, ++seqno) {
std::unique_ptr<Item> item;
auto key = makeStoredDocKey(std::to_string(i));
if (i & 1) {
item.reset(Item::makeDeletedItem(
DeleteSource::Explicit, key, 0, 0, nullptr, 0));
} else {
// Note: this is not really xattr, just checking the datatype is
// preserved on upgrade
const auto body = createXattrValue("value");
item.reset(Item::makeDeletedItem(DeleteSource::Explicit,
key,
0,
0,
"valuable",
8,
body.data(),
body.size(),
PROTOCOL_BINARY_DATATYPE_XATTR));
}
item->setBySeqno(keys + j);
item->setBySeqno(seqno);
kvstore->del(queued_item(std::move(item)));
}
kvstore->commit(flush);
Expand All @@ -276,7 +299,7 @@ void CouchKVStoreTest::collectionsOfflineUpgrade(bool writeAsMadHatter) {
// Use the upgrade tool's objects to run an upgrade
// setup_kv_store will have progressed the rev to .2
Collections::InputCouchFile input({}, data_dir + "/0.couch.2");
CollectionID cid = 500;
CollectionID cid = outputCid;
Collections::OutputCouchFile output({},
data_dir + "/0.couch.3",
cid /*collection-id*/,
Expand All @@ -291,8 +314,7 @@ void CouchKVStoreTest::collectionsOfflineUpgrade(bool writeAsMadHatter) {

auto kvstore2 = KVStoreFactory::create(config2);
auto scanCtx = kvstore2.rw->initBySeqnoScanContext(
std::make_unique<CollectionsOfflineGetCallback>(
std::pair<int, int>{18, 18 + deletedKeys}),
std::make_unique<CollectionsOfflineGetCallback>(cid, deletedRange),
std::make_unique<CollectionsOfflineUpgradeCallback>(cid),
Vbid(0),
1,
Expand All @@ -312,17 +334,37 @@ void CouchKVStoreTest::collectionsOfflineUpgrade(bool writeAsMadHatter) {
auto [success, stats] =
kvstore2.rw->getCollectionStats(*kvstoreContext, cid);
EXPECT_TRUE(success);
EXPECT_EQ(keys - deletedKeys, stats.itemCount);
EXPECT_EQ(keys + deletedKeys, stats.highSeqno);
auto deletedCount = deletedRange.second - deletedRange.first;
EXPECT_EQ(keys - deletedCount, stats.itemCount);
EXPECT_EQ(keys + (deletedCount - 1), stats.highSeqno);
EXPECT_NE(0, stats.diskSize);

// Test that the datafile can be compacted.
// MB-45917
CompactionConfig config{0, 0, true /*purge all tombstones*/, false};
runCompaction(*kvstore2.rw, config);
}

TEST_F(CouchKVStoreTest, CollectionsOfflineUpgrade) {
collectionsOfflineUpgrade(false);
// The key count is large enough to ensure the count uses more than 1 byte
// of leb storage so we validate that leb encode/decode works on this path
collectionsOfflineUpgrade(false, CollectionID{500}, 129, {14, 14 + 18});
}

TEST_F(CouchKVStoreTest, CollectionsOfflineUpgradeMadHatter) {
collectionsOfflineUpgrade(true);
// The key count is large enough to ensure the count uses more than 1 byte
// of leb storage so we validate that leb encode/decode works on this path
collectionsOfflineUpgrade(true, CollectionID{500}, 129, {14, 14 + 18});
}

TEST_F(CouchKVStoreTest, CollectionsOfflineUpgradeMadHatter_MB_45917) {
// Delete all the keys, this would trigger MB-45917.
// Here we upgrade as if from mad-hatter, that means there is no diskSize
// stat in the collections local doc, which then leads to an underflow as
// the wrong file disk size was used in calculating the collection's disk
// size. The target collection could be anything, but do a to=default so
// it more mimics what happens. Finally write 129 keys and delete them all.
collectionsOfflineUpgrade(true, CollectionID::Default, 129, {0, 129});
}

TEST_F(CouchKVStoreTest, OpenHistoricalSnapshot) {
Expand Down

0 comments on commit 0917f06

Please sign in to comment.