maria-developers team mailing list archive

[Sergey Petrunya <psergey@xxxxxxxxxxxx>]

Hello Igor,

Please find the feedback below.

On Mon, Jul 12, 2010 at 07:08:34PM -0700, Igor Babaev wrote:
> Please review this patch for the 5.2 tree.
> 
> Regards,
> Igor.
> 
> -------- Original Message --------
> Subject: [Commits] bzr commit into Mariadb 5.2, with Maria 2.0:maria/5.2
> branch (igor:2821) Bug#604549
> Date: Mon, 12 Jul 2010 18:23:26 -0700 (PDT)
> From: Igor Babaev <igor@xxxxxxxxxxxx>
> Reply-To: maria-developers@xxxxxxxxxxxxxxxxxxx
> To: commits@xxxxxxxxxxx
> 
> #At lp:maria/5.2 based on
> revid:knielsen@xxxxxxxxxxxxxxx-20100709120309-xzhk02q8coq7m6tl
> 
>  2821 Igor Babaev	2010-07-12
>       Fixed bug #604549.
>       There was no error thrown when creating a table with a virtual table
>       computed by an expression returning a row.
>       This caused a crash when inserting into the table.
> 
>       Removed periods at the end of the error messages for virtual columns.
>       Adjusted output in test result files accordingly.
Periods at the end of error messages were apparent for the whole time. Why do
we suddenly decide to remove them now?

> === modified file 'mysql-test/r/plugin.result'
> --- a/mysql-test/r/plugin.result	2010-04-30 20:04:35 +0000
> +++ b/mysql-test/r/plugin.result	2010-07-13 01:23:07 +0000
> @@ -75,9 +75,9 @@ SET SQL_MODE='IGNORE_BAD_TABLE_OPTIONS';
>  #illegal value fixed
>  CREATE TABLE t1 (a int) ENGINE=example ULL=10000000000000000000
> one_or_two='ttt' YESNO=SSS;
>  Warnings:
> -Warning	1651	Incorrect value '10000000000000000000' for option 'ULL'
> -Warning	1651	Incorrect value 'ttt' for option 'one_or_two'
> -Warning	1651	Incorrect value 'SSS' for option 'YESNO'
> +Warning	1652	Incorrect value '10000000000000000000' for option 'ULL'
> +Warning	1652	Incorrect value 'ttt' for option 'one_or_two'
> +Warning	1652	Incorrect value 'SSS' for option 'YESNO'
Why did the warning code change? Is this intentional?

> === modified file 'sql/share/errmsg.txt'
> --- a/sql/share/errmsg.txt	2010-06-01 19:52:20 +0000
> +++ b/sql/share/errmsg.txt	2010-07-13 01:23:07 +0000
> @@ -6211,28 +6211,31 @@ ER_VCOL_BASED_ON_VCOL
>          eng "A computed column cannot be based on a computed column"
> 
>  ER_VIRTUAL_COLUMN_FUNCTION_IS_NOT_ALLOWED
> -        eng "Function or expression is not allowed for column '%s'."
> +        eng "Function or expression is not allowed for column '%s'"
> 
>  ER_DATA_CONVERSION_ERROR_FOR_VIRTUAL_COLUMN
> -        eng "Generated value for computed column '%s' cannot be
> converted to type '%s'."
> +        eng "Generated value for computed column '%s' cannot be
> converted to type '%s'"
> 
>  ER_PRIMARY_KEY_BASED_ON_VIRTUAL_COLUMN
> -        eng "Primary key cannot be defined upon a computed column."
> +        eng "Primary key cannot be defined upon a computed column"
> 
>  ER_KEY_BASED_ON_GENERATED_VIRTUAL_COLUMN
> -        eng "Key/Index cannot be defined on a non-stored computed column."
> +        eng "Key/Index cannot be defined on a non-stored computed column"
> 
>  ER_WRONG_FK_OPTION_FOR_VIRTUAL_COLUMN
> -        eng "Cannot define foreign key with %s clause on a computed
> column."
> +        eng "Cannot define foreign key with %s clause on a computed column"
> 
>  ER_WARNING_NON_DEFAULT_VALUE_FOR_VIRTUAL_COLUMN
> -        eng "The value specified for computed column '%s' in table '%s'
> ignored."
> +        eng "The value specified for computed column '%s' in table '%s'
> ignored"
> 
>  ER_UNSUPPORTED_ACTION_ON_VIRTUAL_COLUMN
> -        eng "'%s' is not yet supported for computed columns."
> +        eng "'%s' is not yet supported for computed columns"
> 
>  ER_CONST_EXPR_IN_VCOL
> -         eng "Constant expression in computed column function is not
> allowed."
> +        eng "Constant expression in computed column function is not
> allowed"
> +
> +ER_ROW_EXPR_FOR_VCOL
> +        eng "Expression for computed column cannot return a row"
>
When one sees this pair of codes ER_CONST_EXPR_IN_VCOL and ER_ROW_EXPR_FOR_VCOL,
one can't help asking himself whether that's the only disallowed expressions,
and if not, do we have error codes for vcol expressions with
- user variables
- subqueries
- SP calls
- etc, etc.
Do we handle such cases at all?

>  ER_DEBUG_SYNC_TIMEOUT
>    eng "debug sync point wait timed out"
> 
> === modified file 'sql/table.cc'
> --- a/sql/table.cc	2010-06-05 14:53:36 +0000
> +++ b/sql/table.cc	2010-07-13 01:23:07 +0000
> @@ -1859,6 +1859,14 @@ bool fix_vcol_expr(THD *thd,
>      goto end;
>    }
>    thd->where= save_where;
> +#if 0
> +#else
> +  if (unlikely(func_expr->result_type() == ROW_RESULT))
> +  {
> +     my_error(ER_ROW_EXPR_FOR_VCOL, MYF(0));
> +     goto end;
> +  }
> +#endif
Please remove #if/#else.

>  #ifdef PARANOID
>    /*
>      Walk through the Item tree checking if all items are valid
> 

BR
 Sergey
-- 
Sergey Petrunia, Software Developer
Monty Program AB, http://askmonty.org
Blog: http://s.petrunia.net/blog