Re: bzr commit into mysql-5.1-bugteam branch (davi:2991) Bug#21704

[Vasil Dimov]

Hi,

On Wed, Jul 01, 2009 at 23:31:26 +0000, Davi Arnaut wrote:
> # At a local mysql-5.1-bugteam repository of davi
> 
>  2991 Davi Arnaut	2009-07-01
>       Bug#21704: Renaming column does not update FK definition
>       
>       The problem is that renaming columns that take part in a
>       foreign key constraint does not cause the definition to be
>       updated to refer to the new column name.
>       
>       The solution is to not allow a column to be renamed if the
>       column is part of a foreign key constraint (either in the
>       referencing or referenced table).
>       
>       This is implemented by iterating over the table's foreign
>       key constraints and checking whether a column that is part
>       of the constraint is being renamed. If a rename is detected,
>       the engine indicates that a full table copy should take place.
>       During the table copy, foreign key checks will prevent the
>       table from being altered.
>      @ mysql-test/include/mix1.inc
>         Add test case for Bug#21704
>      @ mysql-test/include/mtr_warnings.sql
>         Ignore table t3 errors.
>      @ mysql-test/r/innodb_mysql.result
>         Add test case result for Bug#21704
>      @ storage/innobase/handler/ha_innodb.cc
>         Iterate over the foreign key lists checking whether a
>         column is being renamed.
> 
>     modified:
>       mysql-test/include/mix1.inc
>       mysql-test/include/mtr_warnings.sql
>       mysql-test/r/innodb_mysql.result

It is more manageable to put the test in a separate file, the
innodb_mysql test already executes for 45 seconds.

>       storage/innobase/handler/ha_innodb.cc
[...]
> === modified file 'storage/innobase/handler/ha_innodb.cc'
> --- a/storage/innobase/handler/ha_innodb.cc	2009-06-25 09:52:46 +0000
> +++ b/storage/innobase/handler/ha_innodb.cc	2009-07-01 23:31:22 +0000
> @@ -8161,6 +8161,71 @@ innobase_set_cursor_view(
>  }
>  
>  
> +/***********************************************************************
> +Check if column that is participates in a foreign key is being renamed. */
> +static
> +bool
> +column_is_being_renamed(
> +/*=====================*/
> +	TABLE*		table,		/* in: MySQL table */
> +	uint		n_cols,		/* in: number of columns */
> +	const char**	col_names)	/* in: names of the columns */

Hi,

The above comment, function name and parameters names are confusing,
the decorative comment does not match the function name's length and
there is no "out:" comment, describing what the function returns.

> +{
> +	uint j, k;
> +	Field *field;
> +	const char *col_name;
> +
> +	for (j = 0; j < n_cols; j++) {
> +		col_name = col_names[j];
> +		for (k = 0; k < table->s->fields; k++) {
> +			field = table->field[k];
> +			if (!(field->flags & FIELD_IS_RENAMED))
> +				continue;
> +			if (0 == innobase_strcasecmp(field->field_name, col_name))
> +				goto match;
> +		}
> +	}
> +
> +	return FALSE;
> +match:
> +	return TRUE;
> +}

You are mixing bool/true/false with ibool/TRUE/FALSE.

> +/***********************************************************************
> +Check whether a column that participates in a foreign key definition is
> +being renamed. */
> +static
> +bool
> +is_foreign_key_column_renamed(
> +	row_prebuilt_t*	prebuilt,	/* in: InnoDB prebuilt struct */
> +	TABLE*		table)		/* in: MySQL table */
> +{

The comment for this function is essentially the same with the one for
the previous function, but they do different things. There is no
decorative comment and no "out:" comment.

> +	bool ret = FALSE;
> +	dict_foreign_t* foreign;
> +
> +	row_mysql_lock_data_dictionary(prebuilt->trx);
> +
> +	/* Renaming a column in the referenced table. */
> +	foreign = UT_LIST_GET_FIRST(prebuilt->table->referenced_list);
> +	while (foreign != NULL && ret == FALSE) {
> +		ret = column_is_being_renamed(table, foreign->n_fields,
> +						foreign->referenced_col_names);
> +		foreign = UT_LIST_GET_NEXT(referenced_list, foreign);
> +	}
> +
> +	/* Renaming a column in the referencing table. */
> +	foreign = UT_LIST_GET_FIRST(prebuilt->table->foreign_list);
> +	while (foreign != NULL && ret == FALSE) {
> +		ret = column_is_being_renamed(table, foreign->n_fields,
> +						foreign->foreign_col_names);
> +		foreign = UT_LIST_GET_NEXT(foreign_list, foreign);
> +	}
> +
> +	row_mysql_unlock_data_dictionary(prebuilt->trx);
> +
> +	return ret;
> +}
> +
>  bool ha_innobase::check_if_incompatible_data(
>  	HA_CREATE_INFO*	info,
>  	uint		table_changes)
> @@ -8177,6 +8242,16 @@ bool ha_innobase::check_if_incompatible_
>  		return COMPATIBLE_DATA_NO;
>  	}
>  
> +	/* Check if a column participating in a foreign key was renamed.
> +	There is no mechanism for updating InnoDB foreign key definitions. */
> +	if ((UT_LIST_GET_LEN(prebuilt->table->foreign_list) > 0) ||
> +		(UT_LIST_GET_LEN(prebuilt->table->referenced_list) > 0)) {
> +		if (is_foreign_key_column_renamed(prebuilt, table) == TRUE) {
> +
> +			return COMPATIBLE_DATA_NO;
> +		}
> +	}
> +
>  	/* Check that row format didn't change */
>  	if ((info->used_fields & HA_CREATE_USED_ROW_FORMAT) &&
>  		get_row_type() != info->row_type) {
> 
[...]

I have adjusted the patch with the above suggestions and I am attaching
it to this email, please review it and sorry for the delay!

Since this patch touches solely InnoDB files, I have committed it in
the InnoDB 5.1 repository under r5488, so it can take the normal flow of
InnoDB changes (if it is committed directly into the BZR repository an
extra work will have to be done later to sync both trees).

I also indulged in moving the test to a separate file so it is more
manageable and flexible - the innodb_mysql test already takes 45 seconds
to complete.

I will send a new InnoDB 5.1 snapshot, containing r5488 shortly.

Thank you!

-- 
Vasil Dimov
moc.elcaro@stripped        Software Developer @ Oracle/Innobase Oy
gro.DSBeerF@dv                Committer @ FreeBSD.org
gro.d5v@dv                    Home @ Sofia, Bulgaria

Attachment: [text/x-diff] bug21704.diff

Attachment: [text/x-diff]

Attachment: [application/pgp-signature]