Rename method to be used for upgrading Partman 4 to 5 #137
Conversation
Co-authored-by: George Radecke <[email protected]>
| end | ||
|
|
||
| def safe_partman_standardize_partition_naming(table, statement_timeout: 1) | ||
| raise PgHaMigrations::MissingExtensionError, "The pg_partman extension is not installed" unless partman_extension.installed? |
There was a problem hiding this comment.
Technically, this method can be used for partman 4 and 5 (someone might want to rename their tables after upgrading the extension to version 5.x). However, it is a little difficult getting the tables in the right state for testing when partman 5 is installed. Because of that, the partman 5 jobs in the build matrix will just skip all of the tests for this method. So, should we put in the work to get the tests working for partman 5 or should we just put a guard at the top of this method that limits its usage to partman 4?
There was a problem hiding this comment.
I lean toward making the tests work and delivering a functional and confidently safe method for version 5.
There was a problem hiding this comment.
Can you define:
a little difficult
There was a problem hiding this comment.
Partman doesn't allow you to define a custom datetime string in the call to create_parent, so after creating a partition set on partman 5 we would essentially need to reverse the logic of this new method to get it in a state that looks like partman 4
There was a problem hiding this comment.
And I guess an extra note that the actual flow we would like to test is:
- Create table with partman 4
- Upgrade to partman 5
- Run the standardize partman naming method
That's not really feasible since we would have to reach into the postgres container somehow and upgrade the debian package (and then revert it after the test). If we really want to go that route I think we should consider having some kind of separate integration build.
There was a problem hiding this comment.
upgrade the debian package (and then revert it after the test)
Upgrade the partman package, right?
There was a problem hiding this comment.
Yes, we would need to upgrade the package which would enable us to upgrade the extension in the database
| "#{table.inspect}, statement_timeout: #{statement_timeout}) - " \ | ||
| "Renaming #{partitions.size - 1} partition(s)" # excluding default partition | ||
|
|
||
| safely_acquire_lock_for_table(table, part_config.table_name) do |
There was a problem hiding this comment.
Acquiring an AEL on the part_config table seems a bit overkill, but I like the fact that it almost guarantees the update statement will succeed. But maybe this isn't necessary since we're explicitly setting a low statement timeout?
There was a problem hiding this comment.
It's a fast operation. I prefer the safety of the transaction over the negligible chance that we're making someone else wait 200 ms.
There was a problem hiding this comment.
Acquiring an AEL on the part_config table seems a bit overkill, but I like the fact that it almost guarantees the update statement will succeed.
I don't understand the value add. If we're in a transaction, and we should be (I think we should make it explicit if we don't use the adjust_statement_timeout to do it), then we could just do the update first (if the concern is that the update could fail and tank the whole op).
There was a problem hiding this comment.
Yes, we're already in a transaction at this point (safely_acquire_lock_for_table is the thing that does that). It seem incredibly unlikely that something else would be modifying this table at the time the migration is running, so I think I agree that the part_config lock is unnecessary and we can just rely on the statement timeout for safety
There was a problem hiding this comment.
Worth doing the update first?
There was a problem hiding this comment.
I think the statement that is more likely to fail inside the transaction would be the table rename. The update should pretty much always succeed, so 🤷♂️ ? I'm not sure it really matters
Co-authored-by: George Radecke <[email protected]>
|
I think the method is missing from the README. |
Co-authored-by: James Coleman <[email protected]>
Additionally modify the tests to sprinkle in bad table names that will pass our regexes but fail DateTime parsing
Co-authored-by: Ryan Krage <[email protected]>
rkrage
force-pushed
the
partman-migration
branch
from
83f3a0a to
aa2bc32
Compare
Co-authored-by: Ryan Krage <[email protected]>
rkrage
changed the title
3 participants
Partman 5 drops support for keyword intervals. Creating new partition sets with these intervals will raise an error. Existing tables that were created with these intervals will still work in Partman 5, with the exception of
weeklyandquarterly. Thedatetime_stringthat Partman 4 uses for these intervals is no longer supported and therefore partition maintenance will fail after the extension is upgraded to version 5.x. For other intervals, maintenance will continue to work. However, Partman 5 changed the default datetime_string that is used for all intervals (YYYYMMDDandYYYYMMDD_HH24MISS). Compare that to the Parman 4 logic for datetime_string (gross).So, while we could have just addressed
weeklyandquarterlyintervals in this new method, we chose to make it generic such that users can prep all of their partition sets to be consistent with the new Partman 5 naming. The actual method is fairly simple. It does some validation, discovers the list of partitions, delegates to an adapter to calculate their new names, and then transactionally renames the tables and updates thedatetime_string. The meat of this change lies in the adapters and the factory method that determines which adapter to use.References: