Skip to content

Improvement #8161 : Cardinality estimation should use primary record versions only #8162

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.

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

Merged
merged 2 commits into from
Jun 18, 2024

Conversation

hvlad
Copy link
Member

@hvlad hvlad commented Jun 18, 2024

No description provided.

@hvlad hvlad self-assigned this Jun 18, 2024
@hvlad hvlad requested a review from dyemanov June 18, 2024 12:02
@aafemt
Copy link
Contributor

aafemt commented Jun 18, 2024

Do I understand right that your code counts records on only one data page because done become true as soon as recordCount get any non-zero value and inner loop is aborted as well? Old code seems to count records for whole table...

@hvlad
Copy link
Member Author

hvlad commented Jun 18, 2024

Do I understand right that your code counts records on only one data page because done become true as soon as recordCount get any non-zero value and inner loop is aborted as well?

Almost correct

Old code seems to count records for whole table...

No, read it again

src/jrd/dpm.epp Outdated
@@ -245,47 +245,52 @@ double DPM_cardinality(thread_db* tdbb, jrd_rel* relation, const Format* format)

ULONG recordCount = 0, recordLength = 0;

const RelationPages* const relPages = relation->getPages(tdbb);
RelationPages* const relPages = relation->getPages(tdbb);
const vcl* const vector = relPages->rel_pages;
if (vector)
Copy link
Member

Choose a reason for hiding this comment

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

It looks like vector is not used anymore, so maybe just if (relPages->rel_pages) ?
Not sure this check is needed at all, BTW.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done with vector, as for the check - it makes no harm.

@hvlad hvlad merged commit daac348 into B3_0_Release Jun 18, 2024
@hvlad hvlad linked an issue Jun 18, 2024 that may be closed by this pull request
@hvlad hvlad deleted the work/gh-8161 branch September 20, 2024 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cardinality estimation should use primary record versions only
3 participants