Hi, Alexey,
This is good, the behavior is correct, the code is too,
Just few minor details, see below.
On Oct 10, Alexey Botchkov wrote:
> revision-id: 050443323d7 (mariadb-10.6.23-49-g050443323d7)
> parent(s): d891d23ec33
> author: Alexey Botchkov
> committer: Alexey Botchkov
> timestamp: 2025-09-30 02:04:43 +0400
> message:
>
> MDEV-20498 Assertion `table_share->tmp_table != NO_TMP_TABLE || m_lock_type
> == 1' failed upon REBUILD PARTITION.
>
> Now ALTER TABLE DROP column for PARTITIONING columns can't be done
> INPLACE.
> Reorganise checks if it inserts rows from old partition into new that
> wasn't opened for reading. So rows that don't belong there will produce
> an error.
So, two bugs here:
1. ALTER TABLE DROP COLUMN, ADD COLUMN was misplacing rows in
partitions.
2. ALTER TABLE REBUILD PARTITION crashed on misplaced rows.
Could you please make it clear in the commit comment? That you fix two
bugs and what these bugs are. Because they're basically different
unrelated bugs. It would've been even better to split the commit in two
(this is very easy to do with git citool), but it's up to you.
> diff --git a/mysql-test/main/partition.result
> b/mysql-test/main/partition.result
> index f6a656b581f..cc8b8b76d2d 100644
> --- a/mysql-test/main/partition.result
> +++ b/mysql-test/main/partition.result
> @@ -2888,5 +2888,28 @@ a
> 1
> DROP TABLE t1;
> #
> +# MDEV-20498 Assertion `table_share->tmp_table != NO_TMP_TABLE ||
> m_lock_type == 1' failed upon REBUILD PARTITION.
> +#
> +CREATE TABLE t1 (a INT)
> +PARTITION BY LIST (a) (
> +PARTITION p0 VALUES IN (8),
> +PARTITION p1 VALUES IN (4,7,0),
> +PARTITION p2 VALUES IN (9,2,5),
> +PARTITION p3 VALUES IN (3,1,6));
> +INSERT INTO t1 VALUES (8),(5),(4),(0);
> +ALTER TABLE t1 DROP a, ADD a INT NOT NULL DEFAULT 0, algorithm=inplace;
> +ERROR 0A000: ALGORITHM=INPLACE is not supported for this operation. Try
> ALGORITHM=COPY
> +ALTER TABLE t1 DROP a, ADD a INT NOT NULL DEFAULT 0, algorithm=copy;
make it without algorithm=copy here, it should select this
automatically.
> +ALTER TABLE t1 REBUILD PARTITION p0;
CHECK TABLE t1;
would've been more logical here. We want to check if all
rows are in correct partitions, CHECK TABLE does that. REBUILD only does
it for one partition and technically it's a side effect of the rebuild,
not the main functionality.
> +DROP TABLE t1;
> +CREATE TABLE mdev20498 (a INT) ENGINE=myisam PARTITION BY LIST (a)
> +(PARTITION p0 VALUES IN (0), PARTITION p1 VALUES IN (1));
> +INSERT INTO mdev20498 values (0), (0), (1), (1);
> +FLUSH TABLES;
> +CALL mtr.add_suppression("corrupted: row in wrong partition:");
> +ALTER TABLE mdev20498 REBUILD PARTITION p0;
> +ERROR HY000: Found a row in wrong partition (0 != 1) a:1
> +DROP TABLE mdev20498;
> +#
> # End of 10.6 tests
> #
> diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc
> index 3c3c51f92eb..fdba11d03e9 100644
> --- a/sql/ha_partition.cc
> +++ b/sql/ha_partition.cc
> @@ -2221,6 +2221,13 @@ int ha_partition::copy_partitions(ulonglong * const
> copied,
> }
> else
> {
> + if (m_new_file[new_part]->m_lock_type != F_WRLCK)
So, if you'd do
ALTER TABLE mdev20498 REBUILD PARTITION p0,p1;
or
ALTER TABLE mdev20498 REBUILD PARTITION ALL;
then it would've worked? I'm not saying it's wrong, it's probably even
good. I just want to understand what will happen.
Perhaps, add a test case for that?
> + {
> + m_last_part= reorg_part;
> + m_err_rec= table->record[0];
> + result= HA_ERR_ROW_IN_WRONG_PARTITION;
> + goto error;
> + }
> /* Copy record to new handler */
> (*copied)++;
> DBUG_ASSERT(!m_new_file[new_part]->row_logging);
> diff --git a/sql/sql_partition.cc b/sql/sql_partition.cc
> index fac986600b9..b90f8268b23 100644
> --- a/sql/sql_partition.cc
> +++ b/sql/sql_partition.cc
> @@ -4847,6 +4860,39 @@ uint prep_alter_part_table(THD *thd, TABLE *table,
> Alter_info *alter_info,
> DBUG_RETURN(TRUE);
> }
>
> + if (table->part_info && alter_info->partition_flags == 0 &&
> + (alter_info->flags & ALTER_PARSER_DROP_COLUMN))
> + {
> + List_iterator<Alter_drop> drop_it(alter_info->drop_list);
> + Alter_drop *drop;
> +
> + while ((drop= drop_it++))
> + {
> + if (drop->type != Alter_drop::COLUMN)
> + continue;
> +
> + if (check_name_in_fields(table->part_info->part_field_array,
> + drop->name) ||
> + check_name_in_fields(table->part_info->subpart_field_array,
> + drop->name))
> + {
> + /*
> + The ALTER drops column used in partitioning expression.
> + That cannot be done INPLACE.
> + */
> + if (alter_info->algorithm(thd) ==
> + Alter_info::ALTER_TABLE_ALGORITHM_INPLACE)
> + {
> + my_error(ER_ALTER_OPERATION_NOT_SUPPORTED, MYF(0),
> + "ALGORITHM=INPLACE", "ALGORITHM=COPY");
> + DBUG_RETURN(TRUE);
> + }
> + alter_info->set_requested_algorithm(
> + Alter_info::ALTER_TABLE_ALGORITHM_COPY);
No, don't return an error or set ALTER_TABLE_ALGORITHM_COPY here, set
*partition_changed=TRUE;
and mysql_alter_table() will understand what it means.
Also you can put your added check under
if (!*partition_changed)
like in other places in this function.
> + }
> + }
> + }
Regards,
Sergei
Chief Architect, MariaDB Server
and [email protected]
_______________________________________________
developers mailing list -- [email protected]
To unsubscribe send an email to [email protected]
