Increase index flags capacity in ODS14 #8340
Conversation
…4. Avoid private data members in ODS structures. Fix SLONG->ULONG in page numbers in gstat.
| { | ||
| FB_UINT64 irt_transaction; // transaction in progress | ||
| ULONG irt_root; // page number of index root | ||
| }; |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 usingirt_root / irt_transactionand 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.
There was a problem hiding this comment.
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.
| 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, |
There was a problem hiding this comment.
Is it necessary to specify template arg, could it be auto deducted from ctor arguments by compiler ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I've found at least one such usage of AutoSetRestore in remote.cpp - see PortsCleanup::closePorts().
There was a problem hiding this comment.
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.
This PR changes the
irt_repeatlayout to allow more bits for index flags and nativeTraNumberstorage (and also fixes a few related things). Theirt_repeatsize increases from 12 bytes to 16 bytes, but I find it acceptable. We can avoid the union and remove theirt_in_progressflag, but theirt_repeatsize will grow to 24 bytes which doubles the initial storage, so I'm not sure whether it's worth it (as it limits the maximum number of indices per table).I tried to partially synchronize with the
metacachebranch to simplify the merge later, as Alex already needed to have some of these changes in his branch. I'm ready to adjust the patch if needed.Other PRs for ODS14 will follow, so I'd suggest to review them separately but merge together in a batch, to avoid recreating all existing ODS14 databases often due to ongoing ODS changes.