Skip to content

Commit

Permalink
Revert "Merge branch 'catalog_set_cleanup' into dependency_set"
Browse files Browse the repository at this point in the history 
This reverts commit 9e78949, reversing
changes made to a594ecf.
  • Loading branch information
@Tishj
Tishj committed Oct 28, 2023
1 parent aec4a8c commit 903d93d
Show file tree
Hide file tree
Showing 3 changed files with 34 additions and 74 deletions.
42 changes: 20 additions & 22 deletions src/catalog/catalog_set.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,11 @@ class EntryDropper {
public:
//! Both constructor and destructor are privates because they should only be called by DropEntryDependencies
explicit EntryDropper(EntryIndex &entry_index_p) : entry_index(entry_index_p) {
old_deleted = entry_index.GetEntry().deleted;
old_deleted = entry_index.GetEntry()->deleted;
}

~EntryDropper() {
entry_index.GetEntry().deleted = old_deleted;
entry_index.GetEntry()->deleted = old_deleted;
}

private:
Expand Down Expand Up @@ -61,9 +61,9 @@ void CatalogSet::PutEntry(EntryIndex index, unique_ptr<CatalogEntry> catalog_ent
if (entry == entries.end()) {
throw InternalException("Entry with entry index \"%llu\" does not exist", index.GetIndex());
}
catalog_entry->child = entry->second.TakeEntry();
catalog_entry->child = std::move(entry->second.entry);
catalog_entry->child->parent = catalog_entry.get();
entry->second.SetEntry(std::move(catalog_entry));
entry->second.entry = std::move(catalog_entry);
}

bool IsDependencyEntry(CatalogEntry &entry) {
Expand Down Expand Up @@ -116,12 +116,12 @@ bool CatalogSet::CreateEntry(CatalogTransaction transaction, const string &name,
dummy_node->deleted = true;
dummy_node->set = this;

auto entry_index = PutEntry(GenerateCatalogEntryIndex(), std::move(dummy_node));
auto entry_index = PutEntry(current_entry++, std::move(dummy_node));
index = entry_index.GetIndex();
PutMapping(transaction, name, std::move(entry_index));
} else {
index = mapping_value->index.GetIndex();
auto &current = mapping_value->index.GetEntry();
auto &current = *mapping_value->index.GetEntry();
// if it does, we have to check version numbers
if (HasConflict(transaction, current.timestamp)) {
// current version has been written to by a currently active
Expand Down Expand Up @@ -164,7 +164,7 @@ bool CatalogSet::CreateEntry(ClientContext &context, const string &name, unique_
}

optional_ptr<CatalogEntry> CatalogSet::GetEntryInternal(CatalogTransaction transaction, EntryIndex &entry_index) {
auto &catalog_entry = entry_index.GetEntry();
auto &catalog_entry = *entry_index.GetEntry();
// if it does: we have to retrieve the entry and to check version numbers
if (HasConflict(transaction, catalog_entry.timestamp)) {
// current version has been written to by a currently active
Expand Down Expand Up @@ -238,7 +238,7 @@ bool CatalogSet::AlterEntry(CatalogTransaction transaction, const string &name,
if (value->name != original_name) {
auto mapping_value = GetMapping(transaction, value->name);
if (mapping_value && !mapping_value->deleted) {
auto &original_entry = GetEntryForTransaction(transaction, mapping_value->index.GetEntry());
auto &original_entry = GetEntryForTransaction(transaction, *mapping_value->index.GetEntry());
if (!original_entry.deleted) {
entry->UndoAlter(context, alter_info);
string rename_err_msg =
Expand Down Expand Up @@ -290,7 +290,7 @@ void CatalogSet::DropEntryDependencies(CatalogTransaction transaction, EntryInde
EntryDropper dropper(entry_index);

// To correctly delete the object and its dependencies, it temporarily is set to deleted.
entry_index.GetEntry().deleted = true;
entry_index.GetEntry()->deleted = true;

// check any dependencies of this object
D_ASSERT(entry.ParentCatalog().IsDuckCatalog());
Expand Down Expand Up @@ -352,8 +352,8 @@ DuckCatalog &CatalogSet::GetCatalog() {
}

void CatalogSet::CleanupEntry(CatalogEntry &catalog_entry) {
// destroy the backed up entry: it is no longer required
D_ASSERT(catalog_entry.parent);
// destroy the backed up entry: it is no longer required
D_ASSERT(catalog_entry.parent);
lock_guard<mutex> write_lock(catalog.GetWriteLock());
lock_guard<mutex> lock(catalog_lock);
auto parent = catalog_entry.parent;
Expand All @@ -362,7 +362,8 @@ void CatalogSet::CleanupEntry(CatalogEntry &catalog_entry) {
auto mapping_entry = mapping.find(parent->name);
D_ASSERT(mapping_entry != mapping.end());
auto &entry = mapping_entry->second->index.GetEntry();
if (&entry == parent.get()) {
D_ASSERT(entry);
if (entry.get() == parent.get()) {
mapping.erase(mapping_entry);
}
}
Expand All @@ -379,6 +380,7 @@ optional_ptr<MappingValue> CatalogSet::GetLatestMapping(const string &name) {
if (entry != mapping.end()) {
mapping_value = entry->second.get();
} else {

return nullptr;
}
return mapping_value;
Expand Down Expand Up @@ -437,10 +439,6 @@ bool CatalogSet::UseTimestamp(CatalogTransaction transaction, transaction_t time
return false;
}

catalog_entry_t CatalogSet::GenerateCatalogEntryIndex() {
return current_entry++;
}

CatalogEntry &CatalogSet::GetEntryForTransaction(CatalogTransaction transaction, CatalogEntry &current) {
reference<CatalogEntry> entry(current);
while (entry.get().child) {
Expand Down Expand Up @@ -493,7 +491,7 @@ optional_ptr<CatalogEntry> CatalogSet::CreateEntryInternal(CatalogTransaction tr
entry->set = this;
entry->timestamp = 0;

auto entry_index = PutEntry(GenerateCatalogEntryIndex(), std::move(entry));
auto entry_index = PutEntry(current_entry++, std::move(entry));
PutMapping(transaction, name, std::move(entry_index));
mapping[name]->timestamp = 0;
return catalog_entry;
Expand Down Expand Up @@ -549,7 +547,7 @@ optional_ptr<CatalogEntry> CatalogSet::GetEntry(CatalogTransaction transaction,
// we found an entry for this name
// check the version numbers

auto &catalog_entry = mapping_value->index.GetEntry();
auto &catalog_entry = *mapping_value->index.GetEntry();
auto &current = GetEntryForTransaction(transaction, catalog_entry);
const bool entry_is_invalid = !IncludeEntry(include_deleted, current.deleted);
const bool entry_is_out_of_date = current.name != name && !UseTimestamp(transaction, mapping_value->timestamp);
Expand Down Expand Up @@ -597,7 +595,7 @@ void CatalogSet::Undo(CatalogEntry &entry) {
// otherwise we need to update the base entry tables
auto &name = entry.name;
to_be_removed_node.child->SetAsRoot();
mapping[name]->index.SetEntry(std::move(to_be_removed_node.child));
mapping[name]->index.GetEntry() = std::move(to_be_removed_node.child);
entry.parent = nullptr;
}

Expand Down Expand Up @@ -645,7 +643,7 @@ void CatalogSet::Scan(CatalogTransaction transaction, const std::function<void(C
CreateDefaultEntries(transaction, lock);

for (auto &kv : entries) {
auto &entry = kv.second.Entry();
auto &entry = *kv.second.entry.get();
auto &entry_for_transaction = GetEntryForTransaction(transaction, entry);
if (!entry_for_transaction.deleted) {
callback(entry_for_transaction);
Expand All @@ -661,8 +659,8 @@ void CatalogSet::Scan(const std::function<void(CatalogEntry &)> &callback) {
// lock the catalog set
lock_guard<mutex> lock(catalog_lock);
for (auto &kv : entries) {
auto &entry = kv.second.Entry();
auto &commited_entry = GetCommittedEntry(entry);
auto entry = kv.second.entry.get();
auto &commited_entry = GetCommittedEntry(*entry);
if (!commited_entry.deleted) {
callback(commited_entry);
}
Expand Down
35 changes: 6 additions & 29 deletions src/include/duckdb/catalog/catalog_set.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,15 +37,13 @@ class DependencySetCatalogEntry;

typedef unordered_map<CatalogSet *, unique_lock<mutex>> set_lock_map_t;

using catalog_entry_t = idx_t;

struct EntryValue {
public:
EntryValue() = delete;
explicit EntryValue(unique_ptr<CatalogEntry> entry_p) : entry(std::move(entry_p)), reference_count(0) {
EntryValue() {
throw InternalException("EntryValue called without a catalog entry");
}

public:
explicit EntryValue(unique_ptr<CatalogEntry> entry_p) : entry(std::move(entry_p)), reference_count(0) {
}
//! enable move constructors
EntryValue(EntryValue &&other) noexcept {
Swap(other);
Expand All @@ -54,33 +52,13 @@ struct EntryValue {
Swap(other);
return *this;
}
CatalogEntry &Entry() {
return *entry;
}
unique_ptr<CatalogEntry> TakeEntry() {
return std::move(entry);
}
void SetEntry(unique_ptr<CatalogEntry> entry_p) {
entry = std::move(entry_p);
}
void IncreaseRefCount() {
reference_count++;
}
bool DecreaseRefCount() {
D_ASSERT(reference_count != 0);
reference_count--;
return reference_count == 0;
}

private:
void Swap(EntryValue &other) {
std::swap(entry, other.entry);
idx_t count = reference_count;
reference_count = other.reference_count.load();
other.reference_count = count;
}

private:
unique_ptr<CatalogEntry> entry;
atomic<idx_t> reference_count;
};
Expand Down Expand Up @@ -149,7 +127,6 @@ class CatalogSet {
void Verify(Catalog &catalog);

private:
catalog_entry_t GenerateCatalogEntryIndex();
//! Given a root entry, gets the entry valid for this transaction
CatalogEntry &GetEntryForTransaction(CatalogTransaction transaction, CatalogEntry &current);
CatalogEntry &GetCommittedEntry(CatalogEntry &current);
Expand Down Expand Up @@ -180,11 +157,11 @@ class CatalogSet {
//! The catalog lock is used to make changes to the data
mutex catalog_lock;
//! The set of catalog entries
unordered_map<catalog_entry_t, EntryValue> entries;
unordered_map<idx_t, EntryValue> entries;
//! Mapping of string to catalog entry
case_insensitive_map_t<unique_ptr<MappingValue>> mapping;
//! The current catalog entry index
catalog_entry_t current_entry = 0;
idx_t current_entry = 0;
//! The generator used to generate default internal entries
unique_ptr<DefaultGenerator> defaults;
};
Expand Down
31 changes: 8 additions & 23 deletions src/include/duckdb/catalog/mapping_value.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,21 +20,21 @@ struct EntryIndex {
public:
EntryIndex() : catalog(nullptr), index(DConstants::INVALID_INDEX) {
}
EntryIndex(CatalogSet &catalog, catalog_entry_t index) : catalog(&catalog), index(index) {
EntryIndex(CatalogSet &catalog, idx_t index) : catalog(&catalog), index(index) {
auto entry = catalog.entries.find(index);
if (entry == catalog.entries.end()) {
throw InternalException("EntryIndex - Catalog entry not found in constructor!?");
}
catalog.entries.at(index).IncreaseRefCount();
catalog.entries[index].reference_count++;
}
~EntryIndex() {
if (!catalog) {
return;
}
auto entry = catalog->entries.find(index);
D_ASSERT(entry != catalog->entries.end());
const bool reached_zero = entry->second.DecreaseRefCount();
if (reached_zero) {
auto remaining_ref = --entry->second.reference_count;
if (remaining_ref == 0) {
catalog->entries.erase(index);
}
catalog = nullptr;
Expand All @@ -55,29 +55,14 @@ struct EntryIndex {
return *this;
}

private:
EntryValue &GetEntryInternal(catalog_entry_t index) {
unique_ptr<CatalogEntry> &GetEntry() {
auto entry = catalog->entries.find(index);
if (entry == catalog->entries.end()) {
throw InternalException("EntryIndex - Catalog entry not found!?");
}
return entry->second;
}

public:
CatalogEntry &GetEntry() {
auto &entry_value = GetEntryInternal(index);
return entry_value.Entry();
}
unique_ptr<CatalogEntry> TakeEntry() {
auto &entry_value = GetEntryInternal(index);
return entry_value.TakeEntry();
}
void SetEntry(unique_ptr<CatalogEntry> entry) {
auto &entry_value = GetEntryInternal(index);
entry_value.SetEntry(std::move(entry));
return entry->second.entry;
}
catalog_entry_t GetIndex() {
idx_t GetIndex() {
return index;
}
EntryIndex Copy() {
Expand All @@ -93,7 +78,7 @@ struct EntryIndex {

private:
CatalogSet *catalog;
catalog_entry_t index;
idx_t index;
};

struct MappingValue {
Expand Down

0 comments on commit 903d93d

Please sign in to comment.