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.

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

Improve citus_shard_sizes performance in release-11.3 #7051

Merged
merged 1 commit into from
Jul 14, 2023

Conversation

halilozanakgul
Copy link
Contributor

@halilozanakgul halilozanakgul commented Jul 11, 2023

This PR combines the relevant parts of #7003, #7018 and the PR that backports those changes to 11.3, #7050 to merge into the 11.3 release branch.

This PR backports the changes to 11.3 release branch, there is another PR, #7062, for 12.0 release branch and one, #7050, for main branch.

@halilozanakgul halilozanakgul changed the title Improve citus_shard_sizes performance Improve citus_shard_sizes performance ini release-11.3 Jul 11, 2023
@halilozanakgul halilozanakgul changed the title Improve citus_shard_sizes performance ini release-11.3 Improve citus_shard_sizes performance in release-11.3 Jul 11, 2023
@naisila
Copy link
Member

naisila commented Jul 11, 2023

It looks like you are doing two backports in a single PR.

772d194
613cced

I think we should do the backports in two separate PRs, each backport pointing to its commit in main in the description. It's also easier to review. After all, the two commits are 1 month apart.

@codecov
Copy link

codecov bot commented Jul 12, 2023

Codecov Report

Merging #7051 (97bebce) into release-11.3 (c18586c) will increase coverage by 18.54%.
The diff coverage is 100.00%.

❗ Current head 97bebce differs from pull request most recent head aca4adb. Consider uploading reports for the commit aca4adb to get more accurate results

@@                Coverage Diff                @@
##           release-11.3    #7051       +/-   ##
=================================================
+ Coverage         74.70%   93.25%   +18.54%     
=================================================
  Files               687      269      -418     
  Lines            131591    57498    -74093     
=================================================
- Hits              98305    53619    -44686     
+ Misses            33286     3879    -29407     

@halilozanakgul halilozanakgul marked this pull request as ready for review July 12, 2023 12:55
@halilozanakgul halilozanakgul force-pushed the improve_citus_shard_sizes_performance branch from 97bebce to a90a3a7 Compare July 12, 2023 12:57
Copy link
Member

@naisila naisila left a comment

Choose a reason for hiding this comment

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

It looks good to me.
Some notes:

  • make sure your C changes are identical to the ones in main branch
  • would be good to ask another person from the team if it's okay we're backporting two commits in one
  • update the description to say that this PR actually backports to 11.3, and the PR to main branch will also include 11.3-2 in the upgrade/downgrade path.

Copy link
Member

@naisila naisila left a comment

Choose a reason for hiding this comment

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

I tested altering the extension locally, both upgrade and downgrade - it worked fine. But we need to add that to our tests.

i.e. We also need to add ALTER EXTENSION tests to multi_extension.sql file, similar to the other lines in that file. Below is an example for 11.2-2--11.3-1

-- Snapshot of state at 11.2-2
ALTER EXTENSION citus UPDATE TO '11.2-2';
SELECT * FROM multi_extension.print_extension_changes();

-- Test downgrade to 11.2-2 from 11.3-1
ALTER EXTENSION citus UPDATE TO '11.3-1';
ALTER EXTENSION citus UPDATE TO '11.2-2';
-- Should be empty result since upgrade+downgrade should be a no-op
SELECT * FROM multi_extension.print_extension_changes();

-- Snapshot of state at 11.3-1
ALTER EXTENSION citus UPDATE TO '11.3-1';
SELECT * FROM multi_extension.print_extension_changes();

@halilozanakgul halilozanakgul force-pushed the improve_citus_shard_sizes_performance branch from a90a3a7 to aca4adb Compare July 13, 2023 14:54
Copy link
Member

@naisila naisila left a comment

Choose a reason for hiding this comment

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

Lgtm. Thanks.

Please provide a detailed commit description when you merge.

@halilozanakgul halilozanakgul merged commit 21be1af into release-11.3 Jul 14, 2023
108 checks passed
@halilozanakgul halilozanakgul deleted the improve_citus_shard_sizes_performance branch July 14, 2023 14:19
halilozanakgul added a commit that referenced this pull request Jul 14, 2023
This PR moves `citus_shard_sizes` changes from #7003, and #7018 to a new
Citus version 11.3-2

This PR backports the changes to 12.0 release branch, there is another
PR, #7051 for 11.3 release branch, and one, #7050, for main branch.
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.

None yet

2 participants