Hi,
in some place you return TRUE/FALSE and in others 1/0, please be consistent.
Please update the comments of get_partition_id starting at row ~ 2530
ending ~ 2603, since that is currently both duplicated and wrong.
Otherwise OK to push.
/Mattias
Alexey Botchkov wrote:
> #At file:///home/hf/work/mysql_common/38083/
>
> 2743 Alexey Botchkov 2008-09-18
> 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
>
> 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 and subpart_id didn't expect
> an error returned from item->val_int().
> Error checks added.
> modified:
> mysql-test/r/partition.result
> mysql-test/t/partition.test
> sql/opt_range.cc
> sql/partition_info.h
> sql/sql_partition.cc
>
> === modified file 'mysql-test/r/partition.result'
> --- a/mysql-test/r/partition.result 2008-08-15 18:26:25 +0000
> +++ b/mysql-test/r/partition.result 2008-09-18 13:15:10 +0000
> @@ -1637,4 +1637,23 @@ select count(*) from t1, t2 where t1.cre
> count(*)
> 1
> drop table t1, t2;
> +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;
> End of 5.1 tests
>
> === modified file 'mysql-test/t/partition.test'
> --- a/mysql-test/t/partition.test 2008-08-15 18:26:25 +0000
> +++ b/mysql-test/t/partition.test 2008-09-18 13:15:10 +0000
> @@ -1791,4 +1791,26 @@ select count(*) from t1, t2 where t1.cre
>
> drop table t1, t2;
>
> +#
> +# Bug #38083 Error-causing row inserted into partitioned table despite error
> +#
> +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)
> + );
> +
> +-- 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;
> +
> +
> --echo End of 5.1 tests
>
> === modified file 'sql/opt_range.cc'
> --- a/sql/opt_range.cc 2008-08-25 17:18:22 +0000
> +++ b/sql/opt_range.cc 2008-09-18 13:15:10 +0000
> @@ -3145,10 +3145,12 @@ int find_used_partitions(PART_PRUNE_PARA
> ppar->subpart_fields););
> /* Find the subpartition (it's HASH/KEY so we always have one) */
> partition_info *part_info= ppar->part_info;
> - uint32 subpart_id= part_info->get_subpartition_id(part_info);
> + uint32 part_id, subpart_id;
>
> + if (part_info->get_subpartition_id(part_info, &subpart_id))
> + return 0;
> +
> /* Mark this partition as used in each subpartition. */
> - uint32 part_id;
> while ((part_id= ppar->part_iter.get_next(&ppar->part_iter)) !=
> NOT_A_PARTITION_ID)
> {
>
> === modified file 'sql/partition_info.h'
> --- a/sql/partition_info.h 2008-03-16 08:52:37 +0000
> +++ b/sql/partition_info.h 2008-09-18 13:15:10 +0000
> @@ -25,7 +25,8 @@ class partition_info;
> typedef int (*get_part_id_func)(partition_info *part_info,
> uint32 *part_id,
> longlong *func_value);
> -typedef uint32 (*get_subpart_id_func)(partition_info *part_info);
> +typedef int (*get_subpart_id_func)(partition_info *part_info,
> + uint32 *part_info);
>
> struct st_ddl_log_memory_entry;
>
>
> === modified file 'sql/sql_partition.cc'
> --- a/sql/sql_partition.cc 2008-08-11 18:02:03 +0000
> +++ b/sql/sql_partition.cc 2008-09-18 13:15:10 +0000
> @@ -73,10 +73,8 @@ static int get_part_id_charset_func_subp
> static int get_part_part_id_charset_func(partition_info *part_info,
> uint32 *part_id,
> longlong *func_value);
> -static uint32 get_subpart_id_charset_func(partition_info *part_info);
> -int get_partition_id_list(partition_info *part_info,
> - uint32 *part_id,
> - longlong *func_value);
> +static int get_subpart_id_charset_func(partition_info *part_info,
> + uint32 *part_id);
> int get_partition_id_list(partition_info *part_info,
> uint32 *part_id,
> longlong *func_value);
> @@ -119,10 +117,14 @@ int get_partition_id_list_sub_linear_has
> int get_partition_id_list_sub_linear_key(partition_info *part_info,
> uint32 *part_id,
> longlong *func_value);
> -uint32 get_partition_id_hash_sub(partition_info *part_info);
> -uint32 get_partition_id_key_sub(partition_info *part_info);
> -uint32 get_partition_id_linear_hash_sub(partition_info *part_info);
> -uint32 get_partition_id_linear_key_sub(partition_info *part_info);
> +int get_partition_id_hash_sub(partition_info *part_info,
> + uint32 *part_id);
> +int get_partition_id_key_sub(partition_info *part_info,
> + uint32 *part_id);
> +int get_partition_id_linear_hash_sub(partition_info *part_info,
> + uint32 *part_id);
> +int get_partition_id_linear_key_sub(partition_info *part_info,
> + uint32 *part_id);
> static uint32 get_next_partition_via_walking(PARTITION_ITERATOR*);
> static void set_up_range_analysis_info(partition_info *part_info);
> static uint32 get_next_subpartition_via_walking(PARTITION_ITERATOR*);
> @@ -2319,16 +2321,21 @@ 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 +2343,13 @@ 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;
> + if (current_thd->is_error())
> + {
> + *part_id= HA_ERR_NO_PARTITION_FOUND;
> + DBUG_RETURN(TRUE);
> + }
> + DBUG_RETURN(FALSE);
> }
>
>
> @@ -2349,24 +2362,34 @@ 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())
> + {
> + *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);
> }
>
>
> @@ -2640,13 +2663,14 @@ static int get_part_part_id_charset_func
> }
>
>
> -static uint32 get_subpart_id_charset_func(partition_info *part_info)
> +static int get_subpart_id_charset_func(partition_info *part_info,
> + uint32 *part_id)
> {
> int res;
> copy_to_part_field_buffers(part_info->subpart_charset_field_array,
> part_info->subpart_field_buffers,
> part_info->restore_subpart_field_ptrs);
> - res= part_info->get_subpartition_id_charset(part_info);
> + res= part_info->get_subpartition_id_charset(part_info, part_id);
> restore_part_field_pointers(part_info->subpart_charset_field_array,
> part_info->restore_subpart_field_ptrs);
> return res;
> @@ -2666,6 +2690,9 @@ 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)
> @@ -2697,8 +2724,8 @@ int get_partition_id_list(partition_info
> }
> }
> notfound:
> - *part_id= 0;
> - DBUG_RETURN(HA_ERR_NO_PARTITION_FOUND);
> + *part_id= HA_ERR_NO_PARTITION_FOUND;
> + DBUG_RETURN(1);
> }
>
>
> @@ -2813,6 +2840,12 @@ 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())
> + {
> + *part_id= HA_ERR_NO_PARTITION_FOUND;
> + DBUG_RETURN(1);
> + }
> +
> if (part_info->part_expr->null_value)
> {
> *part_id= 0;
> @@ -2970,9 +3003,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 +3012,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 +3054,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 +3079,12 @@ 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 +3153,10 @@ 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 +3178,12 @@ 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);
> }
> @@ -3219,36 +3260,43 @@ int get_partition_id_list_sub_linear_key
> get_partition_id_linear_key_sub
> */
>
> -uint32 get_partition_id_hash_sub(partition_info *part_info)
> +int get_partition_id_hash_sub(partition_info *part_info,
> + uint32 *part_id)
> {
> longlong func_value;
> return get_part_id_hash(part_info->no_subparts, part_info->subpart_expr,
> - &func_value);
> + part_id, &func_value);
> }
>
>
> -uint32 get_partition_id_linear_hash_sub(partition_info *part_info)
> +int get_partition_id_linear_hash_sub(partition_info *part_info,
> + uint32 *part_id)
> {
> longlong func_value;
> return get_part_id_linear_hash(part_info, part_info->no_subparts,
> - part_info->subpart_expr, &func_value);
> + part_info->subpart_expr, part_id,
> + &func_value);
> }
>
>
> -uint32 get_partition_id_key_sub(partition_info *part_info)
> +int get_partition_id_key_sub(partition_info *part_info,
> + uint32 *part_id)
> {
> longlong func_value;
> - return get_part_id_key(part_info->subpart_field_array,
> - part_info->no_subparts, &func_value);
> + *part_id= get_part_id_key(part_info->subpart_field_array,
> + part_info->no_subparts, &func_value);
> + return 0;
> }
>
>
> -uint32 get_partition_id_linear_key_sub(partition_info *part_info)
> +int get_partition_id_linear_key_sub(partition_info *part_info,
> + uint32 *part_id)
> {
> longlong func_value;
> - return get_part_id_linear_key(part_info,
> - part_info->subpart_field_array,
> - part_info->no_subparts, &func_value);
> + *part_id= get_part_id_linear_key(part_info,
> + part_info->subpart_field_array,
> + part_info->no_subparts, &func_value);
> + return 0;
> }
>
>
> @@ -3337,37 +3385,39 @@ static bool check_part_func_bound(Field
> buf A buffer that can be used to evaluate the partition function
> key_info The index object
> key_spec A key_range containing key and key length
> + out:part_id The returned partition id
>
> RETURN VALUES
> - part_id Subpartition id to use
> + TRUE All fields in partition function are set
> + FALSE Not all fields in partition function are set
> +
>
> DESCRIPTION
> Use key buffer to set-up record in buf, move field pointers and
> get the partition identity and restore field pointers afterwards.
> */
>
> -static uint32 get_sub_part_id_from_key(const TABLE *table,uchar *buf,
> - KEY *key_info,
> - const key_range *key_spec)
> +static int get_sub_part_id_from_key(const TABLE *table,uchar *buf,
> + KEY *key_info,
> + const key_range *key_spec,
> + uint32 *part_id)
> {
> uchar *rec0= table->record[0];
> partition_info *part_info= table->part_info;
> - uint32 part_id;
> + int res;
> DBUG_ENTER("get_sub_part_id_from_key");
>
> key_restore(buf, (uchar*)key_spec->key, key_info, key_spec->length);
> if (likely(rec0 == buf))
> - {
> - part_id= part_info->get_subpartition_id(part_info);
> - }
> + res= part_info->get_subpartition_id(part_info, part_id);
> else
> {
> Field **part_field_array= part_info->subpart_field_array;
> set_field_ptr(part_field_array, buf, rec0);
> - part_id= part_info->get_subpartition_id(part_info);
> + res= part_info->get_subpartition_id(part_info, part_id);
> set_field_ptr(part_field_array, rec0, buf);
> }
> - DBUG_RETURN(part_id);
> + DBUG_RETURN(res);
> }
>
> /*
> @@ -3586,7 +3636,13 @@ void get_partition_set(const TABLE *tabl
> else if (part_info->is_sub_partitioned())
> {
> if (part_info->all_fields_in_SPF.is_set(index))
> - sub_part= get_sub_part_id_from_key(table, buf, key_info, key_spec);
> + {
> + if (get_sub_part_id_from_key(table, buf, key_info, key_spec,
> &sub_part))
> + {
> + part_spec->start_part= no_parts;
> + DBUG_VOID_RETURN;
> + }
> + }
> else if (part_info->all_fields_in_PPF.is_set(index))
> {
> if (get_part_id_from_key(table,buf,key_info,
> @@ -3632,7 +3688,14 @@ void get_partition_set(const TABLE *tabl
> else if (part_info->is_sub_partitioned())
> {
> if (check_part_func_bound(part_info->subpart_field_array))
> - sub_part= get_sub_part_id_from_key(table, buf, key_info, key_spec);
> + {
> + if (get_sub_part_id_from_key(table, buf, key_info, key_spec,
> &sub_part))
> + {
> + part_spec->start_part= no_parts;
> + clear_indicator_in_key_fields(key_info);
> + DBUG_VOID_RETURN;
> + }
> + }
> else if (check_part_func_bound(part_info->part_field_array))
> {
> if (get_part_id_from_key(table,buf,key_info,key_spec,&part_part))
> @@ -6836,9 +6899,11 @@ int get_part_iter_for_interval_via_walki
> field->set_null();
> if (is_subpart)
> {
> - part_id= part_info->get_subpartition_id(part_info);
> - init_single_partition_iterator(part_id, part_iter);
> - return 1; /* Ok, iterator initialized */
> + if (!part_info->get_subpartition_id(part_info, &part_id))
> + {
> + init_single_partition_iterator(part_id, part_iter);
> + return 1; /* Ok, iterator initialized */
> + }
> }
> else
> {
> @@ -7007,13 +7072,17 @@ static uint32 get_next_partition_via_wal
> static uint32 get_next_subpartition_via_walking(PARTITION_ITERATOR *part_iter)
> {
> Field *field= part_iter->part_info->subpart_field_array[0];
> + uint32 res;
> if (part_iter->field_vals.cur == part_iter->field_vals.end)
> {
> part_iter->field_vals.cur= part_iter->field_vals.start;
> return NOT_A_PARTITION_ID;
> }
> field->store(part_iter->field_vals.cur++, FALSE);
> - return part_iter->part_info->get_subpartition_id(part_iter->part_info);
> + if (part_iter->part_info->get_subpartition_id(part_iter->part_info,
> + &res))
> + return NOT_A_PARTITION_ID;
> + return res;
> }
>
>
>
>
