Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert the retry applyRecord when loadEdit (#4938) #4980

Merged
merged 5 commits into from
May 24, 2022
Merged

Revert the retry applyRecord when loadEdit (#4938) #4980

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ed0aac2
fix sequence when loadedit
hehechen May 20, 2022
4afd57b
gcInMem should update seq in page directory
JaySon-Huang May 20, 2022
434cb8e
revert the retry applyRecord when loadEdit
hehechen May 23, 2022
9c23380
Merge branch 'release-6.1' into cherry-pick-4938-to-release-6.1
hehechen May 24, 2022
8b246fe
Merge branch 'release-6.1' into cherry-pick-4938-to-release-6.1
ti-chi-bot May 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 4 additions & 18 deletions dbms/src/Storages/Page/V3/PageDirectoryFactory.cpp
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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