-
Notifications
You must be signed in to change notification settings - Fork 120
Fix multiple partition SQL syntax for MySQL #988
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
Conversation
When adding multiple partitions to an existing table, MySQL requires: ALTER TABLE foo ADD PARTITION (PARTITION p1 ..., PARTITION p2 ...) Previously, each AddPartition action generated its own ADD PARTITION clause, which when joined with commas resulted in invalid SQL: ALTER TABLE foo ADD PARTITION (...), ADD PARTITION (...) This fix: - Batches AddPartition actions together in executeActions() - Adds new getAddPartitionsInstructions() method to AbstractAdapter with a default implementation that calls the single partition method - Overrides getAddPartitionsInstructions() in MysqlAdapter to generate correct batched SQL: ADD PARTITION (PARTITION p1, PARTITION p2) - Similarly batches DropPartition actions for efficiency - Adds gatherPartitions() to Plan.php to properly gather partition actions - Includes extensive tests for single/multiple partition add/drop scenarios Refs #986
|
Edge cases analyzed:
Key observations:
|
jamisonbryant
left a comment
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.
This works now for me. Generated query:
ALTER TABLE `foo_partitioned` ADD PARTITION (PARTITION `p2023` VALUES LESS THAN ('2024-01-01'), PARTITION `p2024` VALUES LESS THAN ('2025-01-01'), PARTITION `p2025_01` VALUES LESS THAN ('2025-02-01'), PARTITION `p2025_02` VALUES LESS THAN ('2025-03-01'), PARTITION `p2025_03` VALUES LESS THAN ('2025-04-01'), PARTITION `p2025_04` VALUES LESS THAN ('2025-05-01'), PARTITION `p2025_05` VALUES LESS THAN ('2025-06-01'), PARTITION `p2025_06` VALUES LESS THAN ('2025-07-01'), PARTITION `p2025_07` VALUES LESS THAN ('2025-08-01'), PARTITION `p2025_08` VALUES LESS THAN ('2025-09-01'), PARTITION `p2025_09` VALUES LESS THAN ('2025-10-01'), PARTITION `p2025_10` VALUES LESS THAN ('2025-11-01'), PARTITION `p2025_11` VALUES LESS THAN ('2025-12-01'), PARTITION `p2025_12` VALUES LESS THAN ('2026-01-01'), PARTITION `pmax` VALUES LESS THAN MAXVALUE);
Code changes look good as well. Shall I close my PR?
| $table->addPartitionToExisting('p2023', '2024-01-01') | ||
| ->addPartitionToExisting('p2024', '2025-01-01') | ||
| ->addPartitionToExisting('p2025', '2026-01-01') |
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.
What is the error when one uses addPartitionToExisting() on a new table, and addPartition() to an existing table?
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.
Errors weren't being thrown by the plugin - it's that no SQL (or wrong SQL) is generated for these edge cases.
addPartitionToExisting() on a new table:
When you call create(), the AddPartition action gets filtered out because the table is in tableCreates. The table is created without any partitioning - no SQL generated for the partition.
addPartition() on an existing table:
Two sub-cases:
-
Without calling
partitionBy()first:RuntimeException: 'Must call partitionBy() before addPartition()' -
With
partitionBy()called first: This generates aSetPartitioningaction which produces:
ALTER TABLE existing_table PARTITION BY RANGE (column) (PARTITION p1 VALUES LESS THAN (...))This sets/replaces the entire partition scheme rather than adding a single partition to an existing scheme - wrong SQL for the intent.
| foreach ($partitions as $partition) { | ||
| $instructions->merge($this->getAddPartitionInstructions($table, $partition)); | ||
| } |
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.
Do we need an abstraction for both the collection and the single partition expression? It seems like the collection + single methods are always implemented together and are only used in tandem. Could we have only a single abstract method then?
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.
You're right. Looking at it now:
- The collection method is the only entry point (called from executeActions)
- MySQL's collection method doesn't even use the single method - it uses a private
getAddPartitionSql()helper - The single method only exists for AbstractAdapter's default loop implementation
I can simplify to just the collection methods (getAddPartitionsInstructions / getDropPartitionsInstructions). Each adapter can implement the batching however it needs internally - MySQL with its combined syntax, PostgreSQL with a simple loop. The single-partition logic becomes a private implementation detail rather than part of the abstract contract.
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.
Changes made in #989
|
We can close in favor of #989 then |
Summary
ADD PARTITION (PARTITION p1 ..., PARTITION p2 ...)notADD PARTITION (...), ADD PARTITION (...)Changes Made
executeActions()inAbstractAdapter.phpto batch partition actions togethergetAddPartitionsInstructions()andgetDropPartitionsInstructions()methods toAbstractAdapterMysqlAdapterto generate correct batched SQL syntaxgatherPartitions()toPlan.phpto properly gather partition actions$partitionsproperty and updatedupdatesSequence()/inverseUpdatesSequence()inPlan.phpWhy PR #987 Tests Didn't Catch This
The tests in PR #987 only add/drop a single partition at a time. With a single partition, the generated SQL is valid:
The bug only manifests when adding multiple partitions:
Extensive Tests Added
testAddSinglePartitionToExistingTable- verifies single partition add workstestAddMultiplePartitionsToExistingTable- main test for the fixtestDropSinglePartitionFromExistingTable- verifies single partition drop workstestDropMultiplePartitionsFromExistingTable- verifies batched DROP PARTITIONtestAddMultipleListPartitionsToExistingTable- tests LIST partitioningtestAddPartitionsWithMaxvalue- tests MAXVALUE handling in batched addsRefs #986