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

create_time_partitions and drop_old_time_partitions with less lock. #7540

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

icefairy
Copy link

DESCRIPTION: PR description that will go into the change log, up to 78 characters
from:https://www.postgresql.org/docs/14/sql-createtable.html

Note that creating a partition using PARTITION OF requires taking an ACCESS EXCLUSIVE lock on the parent partitioned table. Likewise, dropping a partition with DROP TABLE requires taking an ACCESS EXCLUSIVE lock on the parent table. It is possible to use ALTER TABLE ATTACH/DETACH PARTITION to perform these operations with a weaker lock, thus reducing interference with concurrent operations on the partitioned table.

@icefairy
Copy link
Author

@microsoft-github-policy-service agree

Copy link

codecov bot commented Feb 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.68%. Comparing base (f424268) to head (7e12c7d).
Report is 22 commits behind head on main.

❗ Current head 7e12c7d differs from pull request most recent head 5683a2b. Consider uploading reports for the commit 5683a2b to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7540      +/-   ##
==========================================
- Coverage   89.68%   89.68%   -0.01%     
==========================================
  Files         282      283       +1     
  Lines       60420    60428       +8     
  Branches     7523     7525       +2     
==========================================
+ Hits        54187    54193       +6     
- Misses       4079     4082       +3     
+ Partials     2154     2153       -1     

@JelteF
Copy link
Contributor

JelteF commented Mar 8, 2024

Thank you for the contribution. This seems like a really nice improvement. For it to actually do something you have to create the relevant 12.2-1.sql files by copying latest.sql. See here for details: https://github.com/citusdata/citus/blob/main/CONTRIBUTING.md#changing-or-creating-functions

@icefairy
Copy link
Author

done

@JelteF
Copy link
Contributor

JelteF commented Apr 10, 2024

Ah, sorry I guess I wasn't clear before. You shouldn't update the old sql files. You should create:

  • src/backend/distributed/sql/udfs/create_time_partitions/12.1-1.sql
  • src/backend/distributed/sql/udfs/drop_old_time_partitions/12.1-1.sql

And #include those in: src/backend/distributed/sql/citus--12.0-1--12.1-1.sql

And do the reverse includes in: src/backend/distributed/sql/downgrades/citus--12.1-1--12.0-1.sql

@icefairy
Copy link
Author

I think it's no need to modify downgrades script yes?

@JelteF
Copy link
Contributor

JelteF commented Apr 29, 2024

I'm sorry, I messed up the version numbers in my previous message. Corrected below:

You shouldn't update the old sql files. You should create:

  • src/backend/distributed/sql/udfs/create_time_partitions/12.2-1.sql
  • src/backend/distributed/sql/udfs/drop_old_time_partitions/12.2-1.sql

And #include those in: src/backend/distributed/sql/citus--12.1-1--12.2-1.sql

And do the reverse includes in: src/backend/distributed/sql/downgrades/citus--12.2-1--12.1-1.sql

@icefairy
Copy link
Author

icefairy commented May 6, 2024

file renamed

@JelteF
Copy link
Contributor

JelteF commented May 6, 2024

You didn't do this part yet:

And #include those in: src/backend/distributed/sql/citus--12.1-1--12.2-1.sql

And do the reverse includes in: src/backend/distributed/sql/downgrades/citus--12.2-1--12.1-1.sql

And you also need to update the latest.sql files to be the same content as the new 12.2-1.sql files.

Finally, could you undo the removal of the trailing new line in these files. No changes should be necessary to these files at all:

  • src/backend/distributed/sql/udfs/create_time_partitions/10.2-1.sql
  • src/backend/distributed/sql/udfs/drop_old_time_partitions/12.0-1.sql

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants