-
Notifications
You must be signed in to change notification settings - Fork 110
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
Fix REFRESH fast path. #720
base: main
Are you sure you want to change the base?
Conversation
This is a cherry-pick commit from commercial version. |
|
||
Form_gp_matview_aux auxform = (Form_gp_matview_aux) GETSTRUCT(mvauxtup); | ||
return (auxform->datastatus == MV_DATA_STATUS_UP_TO_DATE); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That function is differ from MatviewIsGeneralyUpToDate only in predicate || auxform->datastatus == MV_DATA_STATUS_UP_REORGANIZED
Maybe it would be better to create function like matviewCheckForDataStatuses(Oid mvoid, char[] statuses) and not to duplicate steps to get Form_gp_matview_aux structure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HI, @leborchuk thanks for review.
Maybe it would be better to create function like matviewCheckForDataStatuses(Oid mvoid, char[] statuses)
I personally can't agree that, it will make much more complex for function callers to construct a char[] array, then the couple of lines you saved will be back.
And, a lot of codes need to adjust with new function.
@@ -462,3 +462,16 @@ MatviewIsGeneralyUpToDate(Oid mvoid) | |||
return ((auxform->datastatus == MV_DATA_STATUS_UP_TO_DATE) || | |||
(auxform->datastatus == MV_DATA_STATUS_UP_REORGANIZED)); | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line150: namestrcpy(&mvname, get_rel_name(mvoid));
mvname without schema, maybe same mvname
cause confused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch.
The mvname should be with a schema prefix, this is for debug purpose during dev, maybe it could be removed, should be another topic.
If a table is vacuum or clutered, we should not avoid REFRESH for mvs have it. Else, if it's a partition or partitioned now and later, we could not use it for Append Agg. Authored-by: Zhang Mingli [email protected]
7baf081
to
5f03ab1
Compare
If a table is vacuum or clutered, we should not avoid REFRESH for mvs have it.
Else, if it's a partition or partitioned now and later, we could not use it for Append Agg.
Authored-by: Zhang Mingli [email protected]
Fixes #ISSUE_Number
What does this PR do?
Type of Change
Breaking Changes
Test Plan
make installcheck
make -C src/test installcheck-cbdb-parallel
Impact
Performance:
User-facing changes:
Dependencies:
Checklist
Additional Context
[skip ci]
to your PR title. Only use when necessary!