Hi,
You are correct that this actually fixes more than the bug, but please
continue with the fix, and also update sql_partition.cc even a bit further:
Update the comment for get_partition_id() (return value is now 0 - OK,
!= 0 Error, part_id is the out parameter)
and when failing a 'get_part_id_func' function, set *part_id=
HA_ERR_NO_PARTITION_FOUND, or in some places use loc_part_id.
Verify consistent return values / parameters for all 'get_part_id_func'
in sql_partition.cc (for example get_partition_id_list does not comply
to the comment for get_partition_id()) It should return 1 and set
*part_id to HA_ERR_NO_PARTITION_FOUND in 'notfound' label.
remove the duplicate declaration of get_partition_id_list in ha_partition.cc
I have also some proposals below.
Regards
Mattias
Alexey Botchkov wrote:
> #At file:///home/hf/work/mysql_common/38083/
>
> 2683 Alexey Botchkov 2008-08-05
> Bug#38083 Error-causing row inserted into partitioned table despite error
>
> problems are located in the sql_partition.cc where functions calculation
> partition_id
> don't expect error returned from item->val_int().
> Fixed by adding checks to these functions.
> Note - it tries to fix more problems than just the reported bug
>
OK
> per-file comments:
> mysql-test/r/partition_error.result
> Bug#38083 Error-causing row inserted into partitioned table despite error
> test result
>
> mysql-test/t/partition_error.test
> Bug#38083 Error-causing row inserted into partitioned table despite error
> test case
>
> sql/sql_partition.cc
> Bug#38083 Error-causing row inserted into partitioned table despite error
>
> various functions calculationg partition_id didn't expect an error
> returned
> from item->val_int().
> Error checks added.
> modified:
> mysql-test/r/partition_error.result
> mysql-test/t/partition_error.test
> sql/sql_partition.cc
>
> === modified file 'mysql-test/r/partition_error.result'
> --- a/mysql-test/r/partition_error.result 2008-02-25 20:18:50 +0000
> +++ b/mysql-test/r/partition_error.result 2008-08-05 07:40:32 +0000
> @@ -642,3 +642,22 @@ ERROR HY000: This partition function is
> create table t1 (a char(10))
> partition by hash (extractvalue(a,'a'));
> ERROR HY000: This partition function is not allowed
> +set @orig_sql_mode = @@sql_mode;
> +SET SQL_MODE='STRICT_ALL_TABLES,ERROR_FOR_DIVISION_BY_ZERO';
> +CREATE TABLE t1 (c1 INT)
> +PARTITION BY LIST(1 DIV c1) (
> +PARTITION p0 VALUES IN (NULL),
> +PARTITION p1 VALUES IN (1)
> +);
> +INSERT INTO t1 VALUES (0);
> +ERROR 22012: Division by 0
> +SELECT * FROM t1;
> +c1
> +TRUNCATE t1;
> +INSERT INTO t1 VALUES (NULL), (0), (1), (2);
> +ERROR 22012: Division by 0
> +SELECT * FROM t1;
> +c1
> +NULL
> +DROP TABLE t1;
> +set sql_mode= @orig_sql_mode;
>
> === modified file 'mysql-test/t/partition_error.test'
> --- a/mysql-test/t/partition_error.test 2008-02-25 20:18:50 +0000
> +++ b/mysql-test/t/partition_error.test 2008-08-05 07:40:32 +0000
> @@ -826,4 +826,23 @@ partition by range (a + (select count(*)
> create table t1 (a char(10))
> partition by hash (extractvalue(a,'a'));
>
> +#
> +# Bug #38083 Error-causing row inserted into partitioned table despite error
> +#
> +set @orig_sql_mode = @@sql_mode;
UPPER CASE on 'set' -> 'SET'
> +SET SQL_MODE='STRICT_ALL_TABLES,ERROR_FOR_DIVISION_BY_ZERO';
> +CREATE TABLE t1 (c1 INT)
> + PARTITION BY LIST(1 DIV c1) (
> + PARTITION p0 VALUES IN (NULL),
> + PARTITION p1 VALUES IN (1)
> + );
>
> +-- error ER_DIVISION_BY_ZERO
> +INSERT INTO t1 VALUES (0);
> +SELECT * FROM t1;
> +TRUNCATE t1;
> +-- error ER_DIVISION_BY_ZERO
> +INSERT INTO t1 VALUES (NULL), (0), (1), (2);
> +SELECT * FROM t1;
> +DROP TABLE t1;
> +set sql_mode= @orig_sql_mode;
>
UPPER CASE on 'set' -> 'SET'
> === modified file 'sql/sql_partition.cc'
> --- a/sql/sql_partition.cc 2008-07-15 01:43:12 +0000
> +++ b/sql/sql_partition.cc 2008-08-05 07:40:32 +0000
> @@ -2319,16 +2319,19 @@ static uint32 get_part_id_for_sub(uint32
> get_part_id_hash()
> no_parts Number of hash partitions
> part_expr Item tree of hash function
> - out:func_value Value of hash function
> + out:part_id The returned partition id
> + out:func_value Value of hash function
>
> RETURN VALUE
> - Calculated partition id
> + FALSE Success
> + TRUE Failure
> */
>
> inline
> -static uint32 get_part_id_hash(uint no_parts,
> - Item *part_expr,
> - longlong *func_value)
> +static int get_part_id_hash(uint no_parts,
> + Item *part_expr,
> + uint32 *part_id,
> + longlong *func_value)
> {
> longlong int_hash_id;
> DBUG_ENTER("get_part_id_hash");
> @@ -2336,7 +2339,8 @@ static uint32 get_part_id_hash(uint no_p
> *func_value= part_val_int(part_expr);
> int_hash_id= *func_value % no_parts;
>
> - DBUG_RETURN(int_hash_id < 0 ? (uint32) -int_hash_id : (uint32) int_hash_id);
> + *part_id= int_hash_id < 0 ? (uint32) -int_hash_id : (uint32) int_hash_id;
> + DBUG_RETURN(current_thd->is_error());
Why not (so *part_id will not be a valid number):
if (current_thd->is_error())
{
*part_id= HA_ERR_NO_PARTITION_FOUND;
DBUG_RETURN(1);
}
*part_id= int_hash_id < 0 ? (uint32) -int_hash_id : (uint32) int_hash_id;
DBUG_RETURN(0);
> }
>
>
> @@ -2349,24 +2353,30 @@ static uint32 get_part_id_hash(uint no_p
> desired information is given
> no_parts Number of hash partitions
> part_expr Item tree of hash function
> + out:part_id The returned partition id
> out:func_value Value of hash function
>
> RETURN VALUE
> - Calculated partition id
> + FALSE Success
> + TRUE Failure
> */
>
> inline
> -static uint32 get_part_id_linear_hash(partition_info *part_info,
> - uint no_parts,
> - Item *part_expr,
> - longlong *func_value)
> +static int get_part_id_linear_hash(partition_info *part_info,
> + uint no_parts,
> + Item *part_expr,
> + uint32 *part_id,
> + longlong *func_value)
> {
> DBUG_ENTER("get_part_id_linear_hash");
>
> *func_value= part_val_int(part_expr);
> - DBUG_RETURN(get_part_id_from_linear_hash(*func_value,
> - part_info->linear_hash_mask,
> - no_parts));
> + if (current_thd->is_error())
Set *part_id= HA_ERR_NO_PARTITION_FOUND;
> + DBUG_RETURN(TRUE);
> + *part_id= get_part_id_from_linear_hash(*func_value,
> + part_info->linear_hash_mask,
> + no_parts);
> + DBUG_RETURN(FALSE);
> }
>
>
> @@ -2666,6 +2676,8 @@ int get_partition_id_list(partition_info
> bool unsigned_flag= part_info->part_expr->unsigned_flag;
> DBUG_ENTER("get_partition_id_list");
>
> + if (current_thd->is_error())
> + goto notfound;
> if (part_info->part_expr->null_value)
> {
> if (part_info->has_null_value)
> @@ -2813,6 +2825,9 @@ int get_partition_id_range(partition_inf
> bool unsigned_flag= part_info->part_expr->unsigned_flag;
> DBUG_ENTER("get_partition_id_range");
>
> + if (current_thd->is_error())
> + DBUG_RETURN(HA_ERR_NO_PARTITION_FOUND);
> +
> if (part_info->part_expr->null_value)
> {
> *part_id= 0;
> @@ -2970,9 +2985,8 @@ int get_partition_id_hash_nosub(partitio
> uint32 *part_id,
> longlong *func_value)
> {
> - *part_id= get_part_id_hash(part_info->no_parts, part_info->part_expr,
> - func_value);
> - return 0;
> + return get_part_id_hash(part_info->no_parts, part_info->part_expr,
> + part_id, func_value);
> }
>
>
> @@ -2980,9 +2994,8 @@ int get_partition_id_linear_hash_nosub(p
> uint32 *part_id,
> longlong *func_value)
> {
> - *part_id= get_part_id_linear_hash(part_info, part_info->no_parts,
> - part_info->part_expr, func_value);
> - return 0;
> + return get_part_id_linear_hash(part_info, part_info->no_parts,
> + part_info->part_expr, part_id, func_value);
> }
>
>
> @@ -3023,8 +3036,10 @@ int get_partition_id_range_sub_hash(part
> DBUG_RETURN(error);
> }
> no_subparts= part_info->no_subparts;
> - sub_part_id= get_part_id_hash(no_subparts, part_info->subpart_expr,
> - &local_func_value);
> + if (get_part_id_hash(no_subparts, part_info->subpart_expr,
> + &sub_part_id, &local_func_value))
> + DBUG_RETURN(1);
> +
> *part_id= get_part_id_for_sub(loc_part_id, sub_part_id, no_subparts);
> DBUG_RETURN(0);
> }
> @@ -3046,9 +3061,11 @@ int get_partition_id_range_sub_linear_ha
> DBUG_RETURN(error);
> }
> no_subparts= part_info->no_subparts;
> - sub_part_id= get_part_id_linear_hash(part_info, no_subparts,
> - part_info->subpart_expr,
> - &local_func_value);
> + if (get_part_id_linear_hash(part_info, no_subparts,
> + part_info->subpart_expr,
> + &sub_part_id,
> + &local_func_value))
> + DBUG_RETURN(1);
> *part_id= get_part_id_for_sub(loc_part_id, sub_part_id, no_subparts);
> DBUG_RETURN(0);
> }
> @@ -3117,8 +3134,9 @@ int get_partition_id_list_sub_hash(parti
> DBUG_RETURN(error);
> }
> no_subparts= part_info->no_subparts;
> - sub_part_id= get_part_id_hash(no_subparts, part_info->subpart_expr,
> - &local_func_value);
> + if (get_part_id_hash(no_subparts, part_info->subpart_expr,
> + &sub_part_id, &local_func_value))
> + DBUG_RETURN(1);
> *part_id= get_part_id_for_sub(loc_part_id, sub_part_id, no_subparts);
> DBUG_RETURN(0);
> }
> @@ -3140,9 +3158,11 @@ int get_partition_id_list_sub_linear_has
> DBUG_RETURN(error);
> }
> no_subparts= part_info->no_subparts;
> - sub_part_id= get_part_id_linear_hash(part_info, no_subparts,
> - part_info->subpart_expr,
> - &local_func_value);
> + if (get_part_id_linear_hash(part_info, no_subparts,
> + part_info->subpart_expr,
> + &sub_part_id,
> + &local_func_value))
> + DBUG_RETURN(1);
> *part_id= get_part_id_for_sub(loc_part_id, sub_part_id, no_subparts);
> DBUG_RETURN(0);
> }
> @@ -3222,16 +3242,20 @@ int get_partition_id_list_sub_linear_key
> uint32 get_partition_id_hash_sub(partition_info *part_info)
> {
> longlong func_value;
> - return get_part_id_hash(part_info->no_subparts, part_info->subpart_expr,
> - &func_value);
> + uint32 part_id;
> + (void) get_part_id_hash(part_info->no_subparts, part_info->subpart_expr,
> + &part_id, &func_value);
Why not continue to check for errors here? and if error return
HA_ERR_NO_PARTITION_FOUND.
> + return part_id;
> }
>
>
> uint32 get_partition_id_linear_hash_sub(partition_info *part_info)
> {
> longlong func_value;
> - return get_part_id_linear_hash(part_info, part_info->no_subparts,
> - part_info->subpart_expr, &func_value);
> + uint32 part_id;
> + (void) get_part_id_linear_hash(part_info, part_info->no_subparts,
> + part_info->subpart_expr, &part_id,
> &func_value);
Too long line!
Check error message (see above).
> + return part_id;
> }
>
>
>
>
--
Mattias Jonsson
MySQL Software Engineer
Sun Microsystems
www.sun.com, www.mysql.com
Phone: +46 70 245 91 51
