MDEV-15621 Auto add RANGE COLUMNS partitions by interval#5152
MDEV-15621 Auto add RANGE COLUMNS partitions by interval#5152mariadb-YuchenPei wants to merge 3 commits into
Conversation
change p_column_list_val::fixed to a bool remove redundant end label in partition_info::fix_column_value_functions
|
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces range interval partitioning support for DATE, DATETIME, and TIMESTAMP columns, allowing automatic partition creation during DML operations. The changes include parser updates to support interval syntax (including Oracle-style interval functions) and core partitioning logic modifications. The review feedback highlights several critical issues: a missing copy of the interval struct during partition creation that would cause duplicate range values, potential out-of-bounds reads in the parser due to passing non-null-terminated strings to atoi, and multiple potential null-pointer dereferences or crashes in the partitioning and memory allocation paths.
mariadb-YuchenPei
force-pushed
the
bb-main-mdev-15621
branch
2 times, most recently
from
c2470f1 to
4ab6759
Compare
Allow auto partitioning by interval in PARTITION BY RANGE COLUMNS PARTITION BY RANGE COLUMNS (col_name) INTERVAL interval [AUTO] ( PARTITION partition_name VALUES LESS THAN (value) [, PARTITION partition_name VALUES LESS THAN (value) ... ] ) where - col_name is the name of one column of type DATE or DATETIME or TIMESTAMP - at least one partition is supplied, and the highest partition cannot have MAXVALUE range - INTERVAL interval is a positive time interval. it can be mariadb format or oracle NUMTODSINTERVAL/NUMTOYMINTERVAL format. Like versioning, the smallest unit is second, i.e. no subsecond like microsecond. - DATE column cannot have interval with values less than a day When performing DML on such a table, it will first add partitions by the specified interval until the partition covers the current time. Partition addition will not cause an implicit commit like DDL normally does. The partitions are named pN. Otherwise the table behaves exactly the same as a normal RANGE COLUMNS partitioned table. Note that TIMESTAMP is not allowed as a type for PARTITION BY RANGE COLUMNS otherwise.
mariadb-YuchenPei
force-pushed
the
bb-main-mdev-15621
branch
from
4ab6759 to
8c05cb9
Compare
| table->vers_switch_partition(thd, table_list, ot_ctx)) | ||
| (table->vers_switch_partition(thd, table_list, ot_ctx) || | ||
| table->range_interval_check_partition(thd, table_list, ot_ctx)) | ||
| ) |
There was a problem hiding this comment.
Please fix code formatting above.
| { | ||
| err+= str.append(STRING_WITH_LEN(" COLUMNS")); | ||
| err+= add_part_field_list(thd, &str, part_info->part_field_list); | ||
| err+= str.append(STRING_WITH_LEN("INTERVAL ")); |
There was a problem hiding this comment.
Currently table ddls lack visibly lack space:
PARTITION BY RANGE COLUMNS(`c`)INTERVAL 1 DAYmake above " INTERVAL " ?
| if (prep_alter_part_table(thd, table, &alter_info, &create_info, | ||
| &partition_changed, &fast_alter_partition)) | ||
| { | ||
| my_error(ER_INTERNAL_ERROR, MYF(ME_WARNING), |
There was a problem hiding this comment.
Are we going to keep using ER_INTERNAL_ERROR in the final patch (is this what versioning code uses?)
| /* | ||
| TODO(MDEV-15621): need to save reprepare observer and | ||
| reset_n_backup_query_tables_list like in vers_create_partitions? | ||
| */ |
There was a problem hiding this comment.
(for the record: I don't have a clear idea why the above would be needed...)
| case SQLCOM_REPLACE: | ||
| case SQLCOM_REPLACE_SELECT: | ||
| case SQLCOM_UPDATE_MULTI: | ||
| break; |
There was a problem hiding this comment.
Also handle SQLCOM_DELETE_MULTI ?
There was a problem hiding this comment.
Another question: do we have coverage for this scenario:
Row-based replication slave.
We execute CREATE TABLE t1 which creates a table with no partition for today.
Then we read an RBR event inserting a row with today's date.
Will we be able to catch the "opening table with no partition for today" and create the partition?
| my_error(ER_INTERNAL_ERROR, MYF(0), "no range partition specified"); | ||
| return true; | ||
| } | ||
| Item *item= el->get_col_val(0).item_expression; |
There was a problem hiding this comment.
Can this function get more comments and be made easier to read?
Example 1: so we get an item...
and then... do nothing with it.. only some lines after we use it to get the last partition endpoint.
| part_elem->range_value= val->value; | ||
| } | ||
| col_val->fixed= 2; | ||
| col_val->fixed= TRUE; |
There was a problem hiding this comment.
What is the reason for this change?
If we no longer assign 'col_val->fixed= 2' , should the comment in part_column_list_val be updated?
| the ORACLE NUMTODSINTERVAL/NUMTOYMINTERVAL format | ||
| */ | ||
| bool partition_info::set_range_interval(int num, LEX_CSTRING &type, | ||
| bool is_ds, |
There was a problem hiding this comment.
Please add a comment that is_ds is true for days and false for year-month intervals.
4 participants
Reviving #5111