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

Increase index flags capacity in ODS14 #8340

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Increase index flags capacity in ODS14 #8340

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2a00a58
Increase index flags capacity. Store transaction number as plain int6…
dyemanov Dec 4, 2024
428dd18
Revert back private members of irt_repeat
dyemanov Dec 5, 2024
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
Prev Previous commit
Revert back private members of irt_repeat
  • Loading branch information
@dyemanov
dyemanov committed Dec 5, 2024
commit 428dd182f8c20c03ecf850c396a6c67c224823df
22 changes: 11 additions & 11 deletions src/jrd/btr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -924,8 +924,9 @@ bool BTR_delete_index(thread_db* tdbb, WIN* window, USHORT id)
{
index_root_page::irt_repeat* irt_desc = root->irt_rpt + id;
CCH_MARK(tdbb, window);
const PageNumber next(window->win_page.getPageSpaceID(), irt_desc->irt_root);
tree_exists = !irt_desc->isEmpty();
const ULONG rootPage = irt_desc->getRoot();
const PageNumber next(window->win_page.getPageSpaceID(), rootPage);
tree_exists = (rootPage != 0);

// remove the pointer to the top-level index page before we delete it
irt_desc->setEmpty();
Expand Down Expand Up @@ -961,11 +962,12 @@ bool BTR_description(thread_db* tdbb, jrd_rel* relation, index_root_page* root,

const index_root_page::irt_repeat* irt_desc = &root->irt_rpt[id];

if (irt_desc->isEmpty())
const ULONG rootPage = irt_desc->getRoot();
if (!rootPage)
return false;

idx->idx_id = id;
idx->idx_root = irt_desc->irt_root;
idx->idx_root = rootPage;
idx->idx_count = irt_desc->irt_keys;
idx->idx_flags = irt_desc->irt_flags;
idx->idx_runtime_flags = 0;
Expand Down Expand Up @@ -1450,7 +1452,7 @@ void BTR_insert(thread_db* tdbb, WIN* root_window, index_insertion* insertion)
// update the index root page. Oh boy.
index_root_page* root = (index_root_page*) CCH_FETCH(tdbb, root_window, LCK_write, pag_root);

window.win_page = root->irt_rpt[idx->idx_id].irt_root;
window.win_page = root->irt_rpt[idx->idx_id].getRoot();
bucket = (btree_page*) CCH_FETCH(tdbb, &window, LCK_write, pag_index);

if (window.win_page.getPageNum() != idx->idx_root)
Expand Down Expand Up @@ -1500,7 +1502,7 @@ void BTR_insert(thread_db* tdbb, WIN* root_window, index_insertion* insertion)
BUGCHECK(204); // msg 204 index inconsistent
}

window.win_page = root->irt_rpt[idx->idx_id].irt_root;
window.win_page = root->irt_rpt[idx->idx_id].getRoot();
bucket = (btree_page*) CCH_FETCH(tdbb, &window, LCK_write, pag_index);
key.key_length = ret_key.key_length;
memcpy(key.key_data, ret_key.key_data, ret_key.key_length);
Expand Down Expand Up @@ -2392,15 +2394,13 @@ void BTR_selectivity(thread_db* tdbb, jrd_rel* relation, USHORT id, SelectivityL
if (!root)
return;

if (id >= root->irt_count || root->irt_rpt[id].isEmpty())
if (id >= root->irt_count || !root->irt_rpt[id].getRoot())
{
CCH_RELEASE(tdbb, &window);
return;
}

ULONG page = root->irt_rpt[id].irt_root;
fb_assert(page);

ULONG page = root->irt_rpt[id].getRoot();
const bool descending = (root->irt_rpt[id].irt_flags & irt_descending);
const ULONG segments = root->irt_rpt[id].irt_keys;

Expand Down Expand Up @@ -3335,7 +3335,7 @@ static USHORT compress_root(thread_db* tdbb, index_root_page* page)
for (const index_root_page::irt_repeat* const end = root_idx + page->irt_count;
root_idx < end; root_idx++)
{
if (!root_idx->isEmpty())
if (root_idx->getRoot())
{
const USHORT len = root_idx->irt_keys * sizeof(irtd);
p -= len;
Expand Down
33 changes: 18 additions & 15 deletions src/jrd/ods.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,24 +368,26 @@ struct index_root_page
USHORT irt_count; // Number of indices
struct irt_repeat
{
friend class index_root_page; // to allow offset check for private members
private:
union
{
FB_UINT64 irt_transaction; // transaction in progress
ULONG irt_root; // page number of index root
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When it was private there was guarantee of consistent usage of irt_root, irt_transaction and flag irt_in_progress.
Why make it public and open way for errors ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, private data members (and getters/setters) just look unnatural (to me) in ODS structures, IMHO they complicate things. That said, I agree about usage consistency. Now it's also consistent if you check for isEmpty() / inProgress() before using irt_root / irt_transaction and I thought that's enough. I can revert to private members if your disagree.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, private data members (and getters/setters) just look unnatural (to me) in ODS structures, IMHO they complicate things. That said, I agree about usage consistency. Now it's also consistent if you check for isEmpty() / inProgress() before using irt_root / irt_transaction and I thought that's enough. I can revert to private members if your disagree.

For me, it is good case when consistency is over complexity.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Vlad - private members look better to me, specially in a cases when setter may change a few data members in sync and result of getter sometimes depends from more than single field.

Probably with wider than before IRT less such tricks are needed - but for example setting some state in flags that requires root page to be set together with assigning root page value still remains.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, to be changed back.

public:
USHORT irt_desc; // offset to key descriptions
USHORT irt_flags; // index flags
UCHAR irt_keys; // number of keys in index

TraNumber inProgress() const;
bool isEmpty() const;
bool isUsed() const;

void setEmpty();
void setInProgress(TraNumber traNumber);
void clearInProgress(ULONG rootPage);

ULONG getRoot() const;
void setRoot(ULONG rootPage);

bool isUsed() const;
void setEmpty();
} irt_rpt[1];

static_assert(sizeof(struct irt_repeat) == 16, "struct irt_repeat size mismatch");
Expand Down Expand Up @@ -425,16 +427,6 @@ inline constexpr USHORT irt_primary = 16;
inline constexpr USHORT irt_expression = 32;
inline constexpr USHORT irt_condition = 64;

inline TraNumber index_root_page::irt_repeat::inProgress() const
{
return (irt_flags & irt_in_progress) ? irt_transaction : 0;
}

inline bool index_root_page::irt_repeat::isEmpty() const
{
return (irt_flags & irt_in_progress) || (irt_root == 0);
}

inline bool index_root_page::irt_repeat::isUsed() const
{
return (irt_flags & irt_in_progress) || (irt_root != 0);
Expand All @@ -443,15 +435,26 @@ inline bool index_root_page::irt_repeat::isUsed() const
inline void index_root_page::irt_repeat::setEmpty()
{
irt_transaction = 0;
fb_assert(irt_root == 0);
irt_flags = 0;
}

inline TraNumber index_root_page::irt_repeat::inProgress() const
{
return (irt_flags & irt_in_progress) ? irt_transaction : 0;
}

inline void index_root_page::irt_repeat::setInProgress(TraNumber traNumber)
{
irt_transaction = traNumber;
irt_flags |= irt_in_progress;
}

inline ULONG index_root_page::irt_repeat::getRoot() const
{
return (irt_flags & irt_in_progress) ? 0 : irt_root;
}

inline void index_root_page::irt_repeat::setRoot(ULONG rootPage)
{
irt_root = rootPage;
Expand Down
10 changes: 4 additions & 6 deletions src/jrd/validation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1976,12 +1976,10 @@ Validation::RTN Validation::walk_index(jrd_rel* relation, index_root_page* root_
**************************************/
Database* dbb = vdr_tdbb->getDatabase();

if (root_page->irt_rpt[id].isEmpty())
const ULONG page_number = root_page->irt_rpt[id].getRoot();
if (!page_number)
return rtn_ok;

const ULONG page_number = root_page->irt_rpt[id].irt_root;
fb_assert(page_number);

const bool unique = (root_page->irt_rpt[id].irt_flags & (irt_unique | idx_primary));
const bool descending = (root_page->irt_rpt[id].irt_flags & irt_descending);
const bool condition = (root_page->irt_rpt[id].irt_flags & irt_condition);
Expand Down Expand Up @@ -3211,13 +3209,13 @@ Validation::RTN Validation::walk_root(jrd_rel* relation, bool getInfo)
if (!relPages->rel_index_root)
return corrupt(VAL_INDEX_ROOT_MISSING, relation);

index_root_page* page = 0;
index_root_page* page = nullptr;
WIN window(DB_PAGE_SPACE, -1);
fetch_page(!getInfo, relPages->rel_index_root, pag_root, &window, &page);

for (USHORT i = 0; i < page->irt_count; i++)
{
if (page->irt_rpt[i].isEmpty())
if (!page->irt_rpt[i].getRoot())
continue;

MetaName index;
Expand Down
7 changes: 4 additions & 3 deletions src/utilities/gstat/dba.epp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1566,11 +1566,12 @@ static void analyze_index( const dba_rel* relation, dba_idx* index)

const index_root_page* index_root = (const index_root_page*) db_read(relation->rel_index_root);

if (index_root->irt_count <= index->idx_id || index_root->irt_rpt[index->idx_id].isEmpty())
if (index_root->irt_count <= index->idx_id)
return;

ULONG page = index_root->irt_rpt[index->idx_id].irt_root;
fb_assert(page);
ULONG page = index_root->irt_rpt[index->idx_id].getRoot();
if (!page)
return;

// CVC: The two const_cast's for bucket can go away if BTreeNode's functions
// are overloaded for constness. They don't modify bucket and pointer's contents.
Expand Down
Loading