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
33 changes: 17 additions & 16 deletions src/jrd/btr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -924,12 +924,11 @@ 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->getRoot());
tree_exists = (irt_desc->getRoot() != 0);
const PageNumber next(window->win_page.getPageSpaceID(), irt_desc->irt_root);
tree_exists = !irt_desc->isEmpty();

// remove the pointer to the top-level index page before we delete it
irt_desc->setRoot(0);
irt_desc->irt_flags = 0;
irt_desc->setEmpty();
const PageNumber prior = window->win_page;
const USHORT relation_id = root->irt_relation;

Expand Down Expand Up @@ -962,11 +961,11 @@ 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->getRoot() == 0)
if (irt_desc->isEmpty())
return false;

idx->idx_id = id;
idx->idx_root = irt_desc->getRoot();
idx->idx_root = irt_desc->irt_root;
idx->idx_count = irt_desc->irt_keys;
idx->idx_flags = irt_desc->irt_flags;
idx->idx_runtime_flags = 0;
Expand Down Expand Up @@ -1451,7 +1450,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].getRoot();
window.win_page = root->irt_rpt[idx->idx_id].irt_root;
bucket = (btree_page*) CCH_FETCH(tdbb, &window, LCK_write, pag_index);

if (window.win_page.getPageNum() != idx->idx_root)
Expand Down Expand Up @@ -1501,7 +1500,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].getRoot();
window.win_page = root->irt_rpt[idx->idx_id].irt_root;
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 @@ -2124,18 +2123,18 @@ bool BTR_next_index(thread_db* tdbb, jrd_rel* relation, jrd_tra* transaction, in
for (; id < root->irt_count; ++id)
{
const index_root_page::irt_repeat* irt_desc = root->irt_rpt + id;
if (irt_desc->getTransaction() && transaction)
const TraNumber inProgressTrans = irt_desc->inProgress();
if (inProgressTrans && transaction)
{
const TraNumber trans = irt_desc->getTransaction();
CCH_RELEASE(tdbb, window);
const int trans_state = TRA_wait(tdbb, transaction, trans, jrd_tra::tra_wait);
const int trans_state = TRA_wait(tdbb, transaction, inProgressTrans, jrd_tra::tra_wait);
if ((trans_state == tra_dead) || (trans_state == tra_committed))
{
// clean up this left-over index
root = (index_root_page*) CCH_FETCH(tdbb, window, LCK_write, pag_root);
irt_desc = root->irt_rpt + id;

if (irt_desc->getTransaction() == trans)
if (irt_desc->inProgress() == inProgressTrans)
BTR_delete_index(tdbb, window, id);
else
CCH_RELEASE(tdbb, window);
Expand Down Expand Up @@ -2359,7 +2358,7 @@ void BTR_reserve_slot(thread_db* tdbb, IndexCreation& creation)
fb_assert(idx->idx_count <= MAX_UCHAR);
slot->irt_keys = (UCHAR) idx->idx_count;
slot->irt_flags = idx->idx_flags;
slot->setTransaction(transaction->tra_number);
slot->setInProgress(transaction->tra_number);

// Exploit the fact idx_repeat structure matches ODS IRTD one
memcpy(desc, idx->idx_rpt, len);
Expand Down Expand Up @@ -2393,13 +2392,15 @@ void BTR_selectivity(thread_db* tdbb, jrd_rel* relation, USHORT id, SelectivityL
if (!root)
return;

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

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

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

Expand Down Expand Up @@ -3334,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->getRoot())
if (!root_idx->isEmpty())
{
const USHORT len = root_idx->irt_keys * sizeof(irtd);
p -= len;
Expand Down
2 changes: 1 addition & 1 deletion src/jrd/btr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ struct index_desc
ULONG idx_root; // Index root
float idx_selectivity; // selectivity of index
USHORT idx_id;
UCHAR idx_flags;
USHORT idx_flags;
UCHAR idx_runtime_flags; // flags used at runtime, not stored on disk
USHORT idx_primary_index; // id for primary key partner index
USHORT idx_primary_relation; // id for primary key partner relation
Expand Down
2 changes: 1 addition & 1 deletion src/jrd/idx.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ struct ini_idx_t
{
UCHAR ini_idx_index_id;
UCHAR ini_idx_relid;
UCHAR ini_idx_flags;
USHORT ini_idx_flags;
UCHAR ini_idx_segment_count;
USHORT ini_idx_ods;
struct ini_idx_segment_t
Expand Down
2 changes: 1 addition & 1 deletion src/jrd/ini.epp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1636,7 +1636,7 @@ static void store_indices(thread_db* tdbb, USHORT odsVersion)
PAD(relation->rel_name, X.RDB$RELATION_NAME);
PAD(indexName, X.RDB$INDEX_NAME);

X.RDB$UNIQUE_FLAG = index->ini_idx_flags & idx_unique;
X.RDB$UNIQUE_FLAG = (index->ini_idx_flags & idx_unique) ? 1 : 0;
X.RDB$SEGMENT_COUNT = index->ini_idx_segment_count;

if (index->ini_idx_flags & idx_descending)
Expand Down
73 changes: 39 additions & 34 deletions src/jrd/ods.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,39 +368,39 @@ struct index_root_page
USHORT irt_count; // Number of indices
struct irt_repeat
{
private:
friend struct index_root_page; // to allow offset check for private members
ULONG irt_root; // page number of index root if irt_in_progress is NOT set, or
// highest 32 bit of transaction if irt_in_progress is set
ULONG irt_transaction; // transaction in progress (lowest 32 bits)
public:
USHORT irt_desc; // offset to key descriptions
UCHAR irt_keys; // number of keys in index
UCHAR irt_flags;

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

TraNumber getTransaction() const;
void setTransaction(TraNumber traNumber);

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.

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);
void setRoot(ULONG rootPage);

} irt_rpt[1];

static_assert(sizeof(struct irt_repeat) == 12, "struct irt_repeat size mismatch");
static_assert(sizeof(struct irt_repeat) == 16, "struct irt_repeat size mismatch");
static_assert(offsetof(struct irt_repeat, irt_transaction) == 0, "irt_transaction offset mismatch");
static_assert(offsetof(struct irt_repeat, irt_root) == 0, "irt_root offset mismatch");
static_assert(offsetof(struct irt_repeat, irt_transaction) == 4, "irt_transaction offset mismatch");
static_assert(offsetof(struct irt_repeat, irt_desc) == 8, "irt_desc offset mismatch");
static_assert(offsetof(struct irt_repeat, irt_keys) == 10, "irt_keys offset mismatch");
static_assert(offsetof(struct irt_repeat, irt_flags) == 11, "irt_flags offset mismatch");
static_assert(offsetof(struct irt_repeat, irt_flags) == 10, "irt_flags offset mismatch");
static_assert(offsetof(struct irt_repeat, irt_keys) == 12, "irt_keys offset mismatch");
};

static_assert(sizeof(struct index_root_page) == 32, "struct index_root_page size mismatch");
static_assert(sizeof(struct index_root_page) == 40, "struct index_root_page size mismatch");
static_assert(offsetof(struct index_root_page, irt_header) == 0, "irt_header offset mismatch");
static_assert(offsetof(struct index_root_page, irt_relation) == 16, "irt_relation offset mismatch");
static_assert(offsetof(struct index_root_page, irt_count) == 18, "irt_count offset mismatch");
static_assert(offsetof(struct index_root_page, irt_rpt) == 20, "irt_rpt offset mismatch");
static_assert(offsetof(struct index_root_page, irt_rpt) == 24, "irt_rpt offset mismatch");

// key descriptor

Expand All @@ -425,32 +425,37 @@ inline constexpr USHORT irt_primary = 16;
inline constexpr USHORT irt_expression = 32;
inline constexpr USHORT irt_condition = 64;

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

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

inline TraNumber index_root_page::irt_repeat::getTransaction() const
inline bool index_root_page::irt_repeat::isUsed() const
{
return (irt_flags & irt_in_progress) ? ((TraNumber) irt_root << BITS_PER_LONG) | irt_transaction : 0;
return (irt_flags & irt_in_progress) || (irt_root != 0);
}

inline void index_root_page::irt_repeat::setTransaction(TraNumber traNumber)
inline void index_root_page::irt_repeat::setEmpty()
{
irt_root = ULONG(traNumber >> BITS_PER_LONG);
irt_transaction = ULONG(traNumber);
irt_transaction = 0;
irt_flags = 0;
}

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

inline bool index_root_page::irt_repeat::isUsed() const
inline void index_root_page::irt_repeat::setRoot(ULONG rootPage)
{
return (irt_flags & irt_in_progress) || (irt_root != 0);
irt_root = rootPage;
irt_flags &= ~irt_in_progress;
}


Expand Down
27 changes: 14 additions & 13 deletions src/jrd/validation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1956,7 +1956,7 @@ void Validation::walk_generators()
}
}

Validation::RTN Validation::walk_index(jrd_rel* relation, index_root_page& root_page, USHORT id)
Validation::RTN Validation::walk_index(jrd_rel* relation, index_root_page* root_page, USHORT id)
{
/**************************************
*
Expand All @@ -1976,25 +1976,26 @@ Validation::RTN Validation::walk_index(jrd_rel* relation, index_root_page& root_
**************************************/
Database* dbb = vdr_tdbb->getDatabase();

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

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);
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);

temporary_key nullKey, *null_key = 0;
temporary_key nullKey, *null_key = nullptr;
if (unique)
{
index_desc idx;
{
// No need to evaluate index expression and/or condition
AutoSetRestoreFlag<UCHAR> flags(&root_page.irt_rpt[id].irt_flags,
AutoSetRestoreFlag<USHORT> flags(&root_page->irt_rpt[id].irt_flags,
Copy link
Member

Choose a reason for hiding this comment

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

Is it necessary to specify template arg, could it be auto deducted from ctor arguments by compiler ?

Copy link
Member

Choose a reason for hiding this comment

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

It's C++17 feature (https://en.cppreference.com/w/cpp/language/class_template_argument_deduction)
AFAIK, we were not using it, but we could.

Copy link
Member

Choose a reason for hiding this comment

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

I've found at least one such usage of AutoSetRestore in remote.cpp - see PortsCleanup::closePorts().

Copy link
Member Author

@dyemanov dyemanov Dec 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Auto deduction works for irt_expression, but fails for irt_expression | irt_condition, it should be explicitly casted to USHORT to work and it somewhat defeats the idea.

Nevertheless, I'd suggest (outside this PR) to remove AutoSetRestoreFlag here at all and instead add the needed lookup flags to BTR_description. Because the index root page may be locked as LCK_read but we modify data there (even if temporarily) -- this looks hackery.

irt_expression | irt_condition, false);

BTR_description(vdr_tdbb, relation, &root_page, &idx, id);
BTR_description(vdr_tdbb, relation, root_page, &idx, id);
}

null_key = &nullKey;
Expand Down Expand Up @@ -3216,7 +3217,7 @@ Validation::RTN Validation::walk_root(jrd_rel* relation, bool getInfo)

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

MetaName index;
Expand All @@ -3242,7 +3243,7 @@ Validation::RTN Validation::walk_root(jrd_rel* relation, bool getInfo)
if (page->irt_rpt[i].irt_flags & irt_condition)
{
// No need to evaluate index expression
AutoSetRestoreFlag<UCHAR> flag(&page->irt_rpt[i].irt_flags, irt_expression, false);
AutoSetRestoreFlag<USHORT> flag(&page->irt_rpt[i].irt_flags, irt_expression, false);

IdxInfo info;
if (BTR_description(vdr_tdbb, relation, page, &info.m_desc, i))
Expand All @@ -3252,7 +3253,7 @@ Validation::RTN Validation::walk_root(jrd_rel* relation, bool getInfo)
}

output("Index %d (%s)\n", i + 1, index.c_str());
walk_index(relation, *page, i);
walk_index(relation, page, i);
}

release_page(&window);
Expand Down
2 changes: 1 addition & 1 deletion src/jrd/validation.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ class Validation
RTN walk_data_page(jrd_rel*, ULONG, ULONG, UCHAR&);
void walk_database();
void walk_generators();
RTN walk_index(jrd_rel*, Ods::index_root_page&, USHORT);
RTN walk_index(jrd_rel*, Ods::index_root_page*, USHORT);
void walk_pip();
RTN walk_pointer_page(jrd_rel*, ULONG);
RTN walk_record(jrd_rel*, const Ods::rhd*, USHORT, RecordNumber, bool);
Expand Down
Loading
Loading