Skip to content

Commit

Permalink
Revert the retry applyRecord when loadEdit (pingcap#4938) (pingcap#4980)
Browse files Browse the repository at this point in the history 
close pingcap#4937
  • Loading branch information
@ti-chi-bot
ti-chi-bot authored May 24, 2022
1 parent 8c8fee1 commit 4094c38
Show file tree
Hide file tree
Showing 3 changed files with 6 additions and 205 deletions.
22 changes: 4 additions & 18 deletions dbms/src/Storages/Page/V3/PageDirectoryFactory.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,21 +113,13 @@ void PageDirectoryFactory::loadEdit(const PageDirectoryPtr & dir, const PageEntr
if (max_applied_ver < r.version)
max_applied_ver = r.version;

// We can not avoid page id from being reused under some corner situation. Try to do gcInMemEntries
// and apply again to resolve the error.
if (bool ok = applyRecord(dir, r, /*throw_on_error*/ false); unlikely(!ok))
{
dir->gcInMemEntries();
applyRecord(dir, r, /*throw_on_error*/ true);
LOG_FMT_INFO(DB::Logger::get("PageDirectoryFactory"), "resolve from error status done, continue to restore");
}
applyRecord(dir, r);
}
}

bool PageDirectoryFactory::applyRecord(
void PageDirectoryFactory::applyRecord(
const PageDirectoryPtr & dir,
const PageEntriesEdit::EditRecord & r,
bool throw_on_error)
const PageEntriesEdit::EditRecord & r)
{
auto [iter, created] = dir->mvcc_table_directory.insert(std::make_pair(r.page_id, nullptr));
if (created)
Expand Down Expand Up @@ -189,14 +181,8 @@ bool PageDirectoryFactory::applyRecord(
catch (DB::Exception & e)
{
e.addMessage(fmt::format(" [type={}] [page_id={}] [ver={}]", r.type, r.page_id, restored_version));
if (throw_on_error || e.code() != ErrorCodes::PS_DIR_APPLY_INVALID_STATUS)
{
throw e;
}
LOG_FMT_WARNING(DB::Logger::get("PageDirectoryFactory"), "try to resolve error during restore: {}", e.message());
return false;
throw e;
}
return true;
}

void PageDirectoryFactory::loadFromDisk(const PageDirectoryPtr & dir, WALStoreReaderPtr && reader)
Expand Down
5 changes: 2 additions & 3 deletions dbms/src/Storages/Page/V3/PageDirectoryFactory.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,10 +60,9 @@ class PageDirectoryFactory
private:
void loadFromDisk(const PageDirectoryPtr & dir, WALStoreReaderPtr && reader);
void loadEdit(const PageDirectoryPtr & dir, const PageEntriesEdit & edit);
static bool applyRecord(
static void applyRecord(
const PageDirectoryPtr & dir,
const PageEntriesEdit::EditRecord & r,
bool throw_on_error);
const PageEntriesEdit::EditRecord & r);

BlobStore::BlobStats * blob_stats = nullptr;
};
Expand Down
184 changes: 0 additions & 184 deletions dbms/src/Storages/Page/V3/tests/gtest_page_directory.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2214,190 +2214,6 @@ try
}
CATCH

TEST_F(PageDirectoryGCTest, RestoreWithDuplicateID)
try
{
auto restore_from_edit = [](const PageEntriesEdit & edit) {
auto ctx = ::DB::tests::TiFlashTestEnv::getContext();
auto provider = ctx.getFileProvider();
auto path = getTemporaryPath();
PSDiskDelegatorPtr delegator = std::make_shared<DB::tests::MockDiskDelegatorSingle>(path);
PageDirectoryFactory factory;
auto d = factory.createFromEdit(getCurrentTestName(), provider, delegator, edit);
return d;
};

const PageId target_id = 100;
// ========= 1 =======//
// Reuse same id: PUT_EXT/DEL/REF
{
PageEntryV3 entry_50{.file_id = 1, .size = 7890, .tag = 0, .offset = 0x123, .checksum = 0x4567};
PageEntriesEdit edit;
edit.put(50, entry_50);
edit.putExternal(target_id);
edit.del(target_id);
// restart and reuse id=100 as ref to replace put_ext
edit.ref(target_id, 50);

auto restored_dir = restore_from_edit(edit);
auto snap = restored_dir->createSnapshot();
ASSERT_EQ(restored_dir->getNormalPageId(target_id, snap).low, 50);
}
// Reuse same id: PUT_EXT/DEL/PUT
{
PageEntryV3 entry_50{.file_id = 1, .size = 7890, .tag = 0, .offset = 0x123, .checksum = 0x4567};
PageEntryV3 entry_100{.file_id = 100, .size = 7890, .tag = 0, .offset = 0x123, .checksum = 0x4567};
PageEntriesEdit edit;
edit.put(50, entry_50);
edit.putExternal(target_id);
edit.del(target_id);
// restart and reuse id=100 as put to replace put_ext
edit.put(target_id, entry_100);

auto restored_dir = restore_from_edit(edit);
auto snap = restored_dir->createSnapshot();
ASSERT_SAME_ENTRY(restored_dir->get(target_id, snap).second, entry_100);
}

// ========= 1-invalid =======//
// Reuse same id: PUT_EXT/BEING REF/DEL/REF
{
PageEntryV3 entry_50{.file_id = 1, .size = 7890, .tag = 0, .offset = 0x123, .checksum = 0x4567};
PageEntriesEdit edit;
edit.put(50, entry_50);
edit.putExternal(target_id);
edit.ref(101, target_id);
edit.del(target_id);
// restart and reuse id=100 as ref. Should not happen because 101 still ref to 100
edit.ref(target_id, 50);

ASSERT_THROW(restore_from_edit(edit);, DB::Exception);
}
// Reuse same id: PUT_EXT/BEING REF/DEL/PUT
{
PageEntryV3 entry_50{.file_id = 1, .size = 7890, .tag = 0, .offset = 0x123, .checksum = 0x4567};
PageEntryV3 entry_100{.file_id = 100, .size = 7890, .tag = 0, .offset = 0x123, .checksum = 0x4567};
PageEntriesEdit edit;
edit.put(50, entry_50);
edit.putExternal(target_id);
edit.ref(101, target_id);
edit.del(target_id);
// restart and reuse id=100 as put. Should not happen because 101 still ref to 100
edit.put(target_id, entry_100);

ASSERT_THROW(restore_from_edit(edit);, DB::Exception);
}

// ========= 2 =======//
// Reuse same id: PUT/DEL/REF
{
PageEntryV3 entry_50{.file_id = 1, .size = 7890, .tag = 0, .offset = 0x123, .checksum = 0x4567};
PageEntriesEdit edit;
edit.put(50, entry_50);
edit.put(target_id, entry_50);
edit.del(target_id);
// restart and reuse id=100 as ref to replace put
edit.ref(target_id, 50);

auto restored_dir = restore_from_edit(edit);
auto snap = restored_dir->createSnapshot();
ASSERT_EQ(restored_dir->getNormalPageId(target_id, snap).low, 50);
}
// Reuse same id: PUT/DEL/PUT_EXT
{
PageEntryV3 entry_50{.file_id = 1, .size = 7890, .tag = 0, .offset = 0x123, .checksum = 0x4567};
PageEntriesEdit edit;
edit.put(50, entry_50);
edit.put(target_id, entry_50);
edit.del(target_id);
// restart and reuse id=100 as external to replace put
edit.putExternal(target_id);

auto restored_dir = restore_from_edit(edit);
auto snap = restored_dir->createSnapshot();
auto ext_ids = restored_dir->getAliveExternalIds(TEST_NAMESPACE_ID);
ASSERT_EQ(ext_ids.size(), 1);
ASSERT_EQ(*ext_ids.begin(), target_id);
}

// ========= 2-invalid =======//
// Reuse same id: PUT/BEING REF/DEL/REF
{
PageEntryV3 entry_50{.file_id = 1, .size = 7890, .tag = 0, .offset = 0x123, .checksum = 0x4567};
PageEntriesEdit edit;
edit.put(50, entry_50);
edit.put(target_id, entry_50);
edit.ref(101, target_id);
edit.del(target_id);
// restart and reuse id=100 as ref to replace put
edit.ref(target_id, 50);

ASSERT_THROW(restore_from_edit(edit);, DB::Exception);
}
// Reuse same id: PUT/BEING REF/DEL/PUT_EXT
{
PageEntryV3 entry_50{.file_id = 1, .size = 7890, .tag = 0, .offset = 0x123, .checksum = 0x4567};
PageEntriesEdit edit;
edit.put(50, entry_50);
edit.put(target_id, entry_50);
edit.ref(101, target_id);
edit.del(target_id);
// restart and reuse id=100 as external to replace put
edit.putExternal(target_id);

ASSERT_THROW(restore_from_edit(edit);, DB::Exception);
}

// ========= 3 =======//
// Reuse same id: REF/DEL/PUT
{
PageEntryV3 entry_50{.file_id = 1, .size = 7890, .tag = 0, .offset = 0x123, .checksum = 0x4567};
PageEntryV3 entry_100{.file_id = 100, .size = 7890, .tag = 0, .offset = 0x123, .checksum = 0x4567};
PageEntriesEdit edit;
edit.put(50, entry_50);
edit.ref(target_id, 50);
edit.del(target_id);
// restart and reuse id=100 as put to replace ref
edit.put(target_id, entry_100);

auto restored_dir = restore_from_edit(edit);
auto snap = restored_dir->createSnapshot();
ASSERT_SAME_ENTRY(restored_dir->get(target_id, snap).second, entry_100);
}
// Reuse same id: REF/DEL/PUT_EXT
{
PageEntryV3 entry_50{.file_id = 1, .size = 7890, .tag = 0, .offset = 0x123, .checksum = 0x4567};
PageEntriesEdit edit;
edit.put(50, entry_50);
edit.ref(target_id, 50);
edit.del(target_id);
// restart and reuse id=100 as external to replace ref
edit.putExternal(target_id);

auto restored_dir = restore_from_edit(edit);
auto snap = restored_dir->createSnapshot();
auto ext_ids = restored_dir->getAliveExternalIds(TEST_NAMESPACE_ID);
ASSERT_EQ(ext_ids.size(), 1);
ASSERT_EQ(*ext_ids.begin(), target_id);
}
// Reuse same id: REF/DEL/REF another id
{
PageEntryV3 entry_50{.file_id = 1, .size = 7890, .tag = 0, .offset = 0x123, .checksum = 0x4567};
PageEntryV3 entry_51{.file_id = 2, .size = 7890, .tag = 0, .offset = 0x123, .checksum = 0x4567};
PageEntriesEdit edit;
edit.put(50, entry_50);
edit.put(51, entry_51);
edit.ref(target_id, 50);
edit.del(target_id);
// restart and reuse id=target_id as external to replace put
edit.ref(target_id, 51);

auto restored_dir = restore_from_edit(edit);
auto snap = restored_dir->createSnapshot();
ASSERT_EQ(restored_dir->getNormalPageId(target_id, snap).low, 51);
}
}
CATCH

#undef INSERT_ENTRY_TO
#undef INSERT_ENTRY
Expand Down

0 comments on commit 4094c38

Please sign in to comment.