-
Notifications
You must be signed in to change notification settings - Fork 684
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
Allow citus_*_size on index related to a distributed table #7271
Conversation
@microsoft-github-policy-service agree company="Data Bene" |
Hey @c2main, thank you for the contribution. I pushed my suggestions into a separate branch on top of your commits: f81823d. It mostly involves some style changes but more importantly, removes the following block: /*
* After this point, we don't need to keep relationId pointing to the table.
* So we swap with indexId if relevant, only for error log...
*/
if (indexId != InvalidOid)
{
relationId = indexId;
} Because I don't think we need to refer distributed table in the error messages that follows this block earlier. |
Hey @onurctirtir
Thank you for your review and suggestions.
Yeah, I was unsure where to do the indexId/relationId dance. Adding the |
Do you mind applying the suggested changes from mentioned commit? |
Well, I have looked a bit further the code and would like to propose some improvements:
The current fix is working. Improvements can be done in another PR if it's better ? To get an idea of possible future code:
|
This is just to split the diff to fix citusdata#6496: * this first commit for text diff * then the tests * the next one for functionnal diff
Add just some test for index on distributed table.
I just enhanced the existing code to check if the relation is an index belonging to a distributed table. If so the shardId is appended to relation (index) name and the *_size function are executed as before. There is a change in an extern function: `extern StringInfo GenerateSizeQueryOnMultiplePlacements(...)` It's possible to create a new function and deprecate this one later if compatibility is an issue.
And minor indentation fixes, removed old comment, update tests.
@onurctirtir I have cherry-pick your patch and moved down some code. Should be ok now. |
Just noticed #6607 about citus size function improvements, for another PR. |
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.
Shared comments from my (probably) last iteration.
It's hard to make flaky-test detection job passing, so I'll take care of this after this PR becomes fully ready again.
Thank you for your contribution!
Please re-request review when this is the case, thanks. |
…ity.c Co-authored-by: Onur Tirtir <onurcantirtir@gmail.com>
adding more tests suggested by Onur Tirtir, found and fixed a related bug.
per suggestion from Onur.
@onurctirtir I believe I applied all suggested changes. |
error message content has changed (table -> relation)
Codecov Report
@@ Coverage Diff @@
## main #7271 +/- ##
==========================================
- Coverage 93.22% 89.56% -3.66%
==========================================
Files 275 275
Lines 59527 59588 +61
Branches 0 7425 +7425
==========================================
- Hits 55495 53371 -2124
- Misses 4032 4089 +57
- Partials 0 2128 +2128 |
7a5d311
to
09d372a
Compare
09d372a
to
4feb747
Compare
I just enhanced the existing code to check if the relation is an index belonging to a distributed table. If so the shardId is appended to relation (index) name and the *_size function are executed as before. There is a change in an extern function: `extern StringInfo GenerateSizeQueryOnMultiplePlacements(...)` It's possible to create a new function and deprecate this one later if compatibility is an issue. Fixes #6496. DESCRIPTION: Allows using Citus size functions on distributed tables indexes. --------- Co-authored-by: Onur Tirtir <onurcantirtir@gmail.com>
I just enhanced the existing code to check if the relation is an index belonging to a distributed table.
If so the shardId is appended to relation (index) name and the *_size function are executed as before.
There is a change in an extern function:
extern StringInfo GenerateSizeQueryOnMultiplePlacements(...)
It's possible to create a new function and deprecate this one later if compatibility is an issue.
Fixes #6496.
DESCRIPTION: Allows using Citus size functions on distributed tables indexes.