review of patch for Bug#27175: "invalid column attributes in create/alter table"

[Chad MILLER]

Martin,

Inside is a Doxygen clarification that may interest everyone.

All, Martin created Bug#27175 to track this problem, and attached  
files to that.   Review of patch from  http://bugs.mysql.com/file.php? 
id=5953 :


> *** ./sql/sql_lex.h	Thu Mar  8 17:29:58 2007
> --- ../mysql-5.0.bk/./sql/sql_lex.h	Thu Mar 15 00:21:50 2007
> ***************
> *** 985,992 ****
> --- 985,1041 ----
>       MIN(i) in the WHERE clause is not allowed in the opposite to  
> MIN(i)
>       in the HAVING clause. Due to possible nesting of select  
> construct
>       the variable can contain 0 or 1 for each nest level.
>     */
> +
> +   /**
> +      @brief   BitMask of Bits used in "type" (only used in sql_yacc)
> +      @details Keep track of each bit which has been explicit set  
> to 0 or 1.
> +               This allows to detect conlflicts, between user options
> +               Otherwise a user cut specify "c int  PRIMARY KEY   
> NULL"
> +   */
> +   ulong type_explicit;


Extraneous comma.  "conflicts"  "cut" -> "could"?  Punctuation missing.

Our Doxygen guidelines are new and (AFAIK) we don't have any options  
recommended, and I advocate that we make "JAVADOC_AUTOBRIEF" on, and  
so this could be
   /**
     BitMask of Bits used in "type" (only used in sql_yacc).  Keep  
track of
     each bit which has been explicit set to 0 or 1.  This allows to  
detect
     conflicts between user options.  Otherwise a user could specify
     "c int  PRIMARY KEY  NULL".
   */
which I think you'll agree is prettier.


> +
> +   /**
> +      @brief   Set Bits in "type" (only used in sql_yacc)
> +      @details Raise an error if the bit is explicitly unset
> +   */
> +   inline bool set_type_bits(ulong bits) {

Brace on following line, according to our standards.

> +     if ((type_explicit & bits) != (type_explicit & bits & type))
> +       return 0; // some of the bits we want to set, are  
> explicitly unset
> +     type|= bits;
> +     type_explicit|= bits;
> +     return 1;
> +   }
> +
> +   /**
> +      @brief   Unset Bits in "type" (only used in sql_yacc)
> +      @details Raise an error if the bit is explicitly set
> +   */
> +   inline bool unset_type_bits(ulong bits) {
> +     if ((type_explicit & bits) != (type_explicit & bits & (~type)))
> +       return 0; // some of the bits we want to unset, are  
> explicitly set

Awkward and confusing comment.  Please rephrase it.

> +     type&= ~bits;
> +     type_explicit|= bits;
> +     return 1;
> +   }
> +
> +   /**
> +      @brief   BitMask of attributes used in column def (only used  
> in sql_yacc)
> +   */
> +   ulong attrib_used;
> +   /**
> +      @brief   Check if attribute was used before (only used in  
> sql_yacc)
> +      @details Check if an attribute is given more than once and
> +               set an error.
> +               "c char(1) default 'a' default 'b'"
> +   */
> +   inline bool test_attrib_used(ulong attrib) {

Brace beneath first "i" of inline.

> +     if (attrib_used & attrib) return 0;

"return" on a line by itself, please.

> +     attrib_used|= attrib;
> +     return 1;
> +   }
> +
>     nesting_map allow_sum_func;
>     enum_sql_command sql_command, orig_sql_command;
>     thr_lock_type lock_option;
>     enum SSL_type ssl_type;			/* defined in violite.h */
> *** ./sql/sql_yacc.yy	Wed Mar  7 09:24:42 2007
> --- ../mysql-5.0.bk/./sql/sql_yacc.yy	Thu Mar 15 12:15:20 2007
> ***************
> *** 1044,1052 ****
>   %type <string>
>   	text_string opt_gconcat_separator
>
>   %type <num>
> ! 	type int_type real_type order_dir lock_option
>   	udf_type if_exists opt_local opt_table_options table_options
>           table_option opt_if_not_exists opt_no_write_to_binlog
>           delete_option opt_temporary all_or_any opt_distinct
>           opt_ignore_leaves fulltext_options spatial_type union_option
> --- 1044,1052 ----
>   %type <string>
>   	text_string opt_gconcat_separator
>
>   %type <num>
> ! 	type type2 int_type real_type order_dir lock_option

"type2" isn't descriptive enough.  Surely there's a better name.

>   	udf_type if_exists opt_local opt_table_options table_options
>           table_option opt_if_not_exists opt_no_write_to_binlog
>           delete_option opt_temporary all_or_any opt_distinct
>           opt_ignore_leaves fulltext_options spatial_type union_option
> ***************
> *** 3140,3147 ****
> --- 3140,3157 ----
>   	    MYSQL_YYABORT;
>   	};
>
>   type:
> +         {
> +           LEX *lex=Lex;
> +           lex->type_explicit= 0;
> +           lex->attrib_used= 0;
> +         }
> +         type2
> +         { $$ = $2; }
> +         ;
> +
> + type2:
>   	int_type opt_len field_options	{ $$=$1; }
>   	| real_type opt_precision field_options { $$=$1; }
>   	| FLOAT_SYM float_options field_options { $$=FIELD_TYPE_FLOAT; }
>   	| BIT_SYM			{ Lex->length= (char*) "1";
> ***************
> *** 3187,3195 ****
>               {
>                 /*
>                   Unlike other types TIMESTAMP fields are NOT NULL  
> by default.
>                 */
> !               Lex->type|= NOT_NULL_FLAG;
>   	      $$=FIELD_TYPE_TIMESTAMP;
>               }
>   	   }
>   	| DATETIME			{ $$=FIELD_TYPE_DATETIME; }
> --- 3197,3205 ----
>               {
>                 /*
>                   Unlike other types TIMESTAMP fields are NOT NULL  
> by default.
>                 */
> !               Lex->type|= NOT_NULL_FLAG; // Do not use set_type_bits
>   	      $$=FIELD_TYPE_TIMESTAMP;
>               }
>   	   }
>   	| DATETIME			{ $$=FIELD_TYPE_DATETIME; }

Okay.  For cases like this, try to trick your "diff" into emitting  
more lines of context.  The comment saved me here.

> ***************
> *** 3233,3241 ****
>   	| LONG_SYM opt_binary		{ $$=FIELD_TYPE_MEDIUM_BLOB; }
>   	| SERIAL_SYM
>   	  {
>   	    $$=FIELD_TYPE_LONGLONG;
> ! 	    Lex->type|= (AUTO_INCREMENT_FLAG | NOT_NULL_FLAG |  
> UNSIGNED_FLAG |
>   		         UNIQUE_FLAG);
>   	  }
>   	;
>
> --- 3243,3251 ----
>   	| LONG_SYM opt_binary		{ $$=FIELD_TYPE_MEDIUM_BLOB; }
>   	| SERIAL_SYM
>   	  {
>   	    $$=FIELD_TYPE_LONGLONG;
> ! 	    Lex->set_type_bits(AUTO_INCREMENT_FLAG | NOT_NULL_FLAG |  
> UNSIGNED_FLAG |
>   		         UNIQUE_FLAG);
>   	  }
>           ;
>

Okay.

> ***************
> *** 3308,3318 ****
>   	field_opt_list field_option {}
>   	| field_option {};
>
>   field_option:
> ! 	SIGNED_SYM	{}
> ! 	| UNSIGNED	{ Lex->type|= UNSIGNED_FLAG;}
> ! 	| ZEROFILL	{ Lex->type|= UNSIGNED_FLAG | ZEROFILL_FLAG; };
>
>   opt_len:
>   	/* empty */	{ Lex->length=(char*) 0; } /* use default length */
>   	| '(' NUM ')'	{ Lex->length= $2.str; };
> --- 3318,3336 ----
>   	field_opt_list field_option {}
>   	| field_option {};
>
>   field_option:
> ! 	SIGNED_SYM {
> !             MYSQL_YYABORT_UNLESS(!(Lex->type_explicit &  
> UNSIGNED_FLAG) &&  Lex->unset_type_bits(UNSIGNED_FLAG));
> !           }

Okay.

> ! 	| UNSIGNED {
> !             MYSQL_YYABORT_UNLESS(Lex->test_attrib_used 
> (UNSIGNED_FLAG));
> !             MYSQL_YYABORT_UNLESS(Lex->set_type_bits(UNSIGNED_FLAG));
> !           }

Okay.

> ! 	| ZEROFILL {
> !             MYSQL_YYABORT_UNLESS(Lex->test_attrib_used 
> (ZEROFILL_FLAG));
> !             MYSQL_YYABORT_UNLESS(Lex->set_type_bits(UNSIGNED_FLAG  
> | ZEROFILL_FLAG));
> !           };
>
>   opt_len:
>   	/* empty */	{ Lex->length=(char*) 0; } /* use default length */
>   	| '(' NUM ')'	{ Lex->length= $2.str; };

Okay.

> ***************
> *** 3329,3367 ****
>   	opt_attribute_list attribute {}
>   	| attribute;
>
>   attribute:
> ! 	NULL_SYM	  { Lex->type&= ~ NOT_NULL_FLAG; }
> ! 	| not NULL_SYM	  { Lex->type|= NOT_NULL_FLAG; }
> ! 	| DEFAULT now_or_signed_literal { Lex->default_value=$2; }
>   	| ON UPDATE_SYM NOW_SYM optional_braces
> !           { Lex->on_update_value= new Item_func_now_local(); }
> ! 	| AUTO_INC	  { Lex->type|= AUTO_INCREMENT_FLAG | NOT_NULL_FLAG; }
>   	| SERIAL_SYM DEFAULT VALUE_SYM
>   	  {
>   	    LEX *lex=Lex;
> ! 	    lex->type|= AUTO_INCREMENT_FLAG | NOT_NULL_FLAG | UNIQUE_FLAG;
>   	    lex->alter_info.flags|= ALTER_ADD_INDEX;
>   	  }
>   	| opt_primary KEY_SYM
>   	  {
>   	    LEX *lex=Lex;
> ! 	    lex->type|= PRI_KEY_FLAG | NOT_NULL_FLAG;
>   	    lex->alter_info.flags|= ALTER_ADD_INDEX;
>   	  }
>   	| UNIQUE_SYM	
>   	  {
>   	    LEX *lex=Lex;
> ! 	    lex->type|= UNIQUE_FLAG;
>   	    lex->alter_info.flags|= ALTER_ADD_INDEX;
>   	  }
>   	| UNIQUE_SYM KEY_SYM
>   	  {
>   	    LEX *lex=Lex;
>   	    lex->type|= UNIQUE_KEY_FLAG;
>   	    lex->alter_info.flags|= ALTER_ADD_INDEX;
>   	  }
> ! 	| COMMENT_SYM TEXT_STRING_sys { Lex->comment= $2; }
>   	| COLLATE_SYM collation_name
>   	  {
>   	    if (Lex->charset && !my_charset_same(Lex->charset,$2))
>   	    {
> --- 3347,3416 ----
>   	opt_attribute_list attribute {}
>   	| attribute;
>
>   attribute:
> ! 	NULL_SYM
> !           {
> ! 	    LEX *lex=Lex;
> !             MYSQL_YYABORT_UNLESS(lex->test_attrib_used 
> (NOT_NULL_FLAG));
> !             MYSQL_YYABORT_UNLESS(lex->unset_type_bits 
> (NOT_NULL_FLAG));

Hmm, whitespace here.  Please align.

> !           }
> ! 	| not NULL_SYM
> !           {
> !  	    LEX *lex=Lex;
> !             MYSQL_YYABORT_UNLESS(lex->test_attrib_used 
> (NOT_NULL_FLAG));
> !             MYSQL_YYABORT_UNLESS(lex->set_type_bits(NOT_NULL_FLAG));
> !           }
> ! 	| DEFAULT now_or_signed_literal
> !           {
> !  	    LEX *lex=Lex;
> !             MYSQL_YYABORT_UNLESS(lex->test_attrib_used 
> (COL_HAS_DEFAULT));
> !             lex->default_value=$2;
> !           }
>   	| ON UPDATE_SYM NOW_SYM optional_braces
> !           {
> !  	    LEX *lex=Lex;
> !             MYSQL_YYABORT_UNLESS(lex->on_update_value == 0);
> !             lex->on_update_value= new Item_func_now_local();
> !           }
> ! 	| AUTO_INC
> !           {
> !  	    LEX *lex=Lex;
> !             MYSQL_YYABORT_UNLESS(lex->test_attrib_used 
> (AUTO_INCREMENT_FLAG));
> !             MYSQL_YYABORT_UNLESS(lex->set_type_bits 
> (AUTO_INCREMENT_FLAG | NOT_NULL_FLAG));
> !           }
>   	| SERIAL_SYM DEFAULT VALUE_SYM
>   	  {
>   	    LEX *lex=Lex;
> !             MYSQL_YYABORT_UNLESS(lex->test_attrib_used 
> (COL_SERIAL_VAL));
> !             MYSQL_YYABORT_UNLESS(lex->set_type_bits 
> (AUTO_INCREMENT_FLAG | NOT_NULL_FLAG | UNIQUE_FLAG));
>               lex->alter_info.flags|= ALTER_ADD_INDEX;
>   	  }
>   	| opt_primary KEY_SYM
>   	  {
>   	    LEX *lex=Lex;
> !             MYSQL_YYABORT_UNLESS(lex->test_attrib_used 
> (PRI_KEY_FLAG));
> !             MYSQL_YYABORT_UNLESS(lex->set_type_bits(PRI_KEY_FLAG  
> | NOT_NULL_FLAG));
>   	    lex->alter_info.flags|= ALTER_ADD_INDEX;
>   	  }
>   	| UNIQUE_SYM	
>   	  {
>   	    LEX *lex=Lex;
> !             MYSQL_YYABORT_UNLESS(lex->test_attrib_used 
> (UNIQUE_FLAG));
> !             MYSQL_YYABORT_UNLESS(lex->set_type_bits(UNIQUE_FLAG));
>   	    lex->alter_info.flags|= ALTER_ADD_INDEX;
>   	  }
>   	| UNIQUE_SYM KEY_SYM
>   	  {
>   	    LEX *lex=Lex;
> +             MYSQL_YYABORT_UNLESS(lex->test_attrib_used 
> (UNIQUE_FLAG));
>   	    lex->type|= UNIQUE_KEY_FLAG;
>   	    lex->alter_info.flags|= ALTER_ADD_INDEX;
>   	  }
> ! 	| COMMENT_SYM TEXT_STRING_sys {
> !             MYSQL_YYABORT_UNLESS(Lex->test_attrib_used 
> (COL_HAS_COMMENT));
> !             Lex->comment= $2;
> !           }
>   	| COLLATE_SYM collation_name
>   	  {
>   	    if (Lex->charset && !my_charset_same(Lex->charset,$2))
>   	    {

I haven't fully checked that these are right.

> *** ./include/mysql_com.h	Wed Mar 14 11:05:36 2007
> --- ../mysql-5.0.bk/./include/mysql_com.h	Wed Mar 14 15:28:56 2007
> ***************
> *** 94,101 ****
> --- 94,104 ----
>   #define PART_KEY_FLAG	16384		/* Intern; Part of some key */
>   #define GROUP_FLAG	32768		/* Intern: Group field */
>   #define UNIQUE_FLAG	65536		/* Intern: Used by sql_yacc */
>   #define BINCMP_FLAG	131072		/* Intern: Used by sql_yacc */
> + #define COL_HAS_DEFAULT	1<<18		/* Intern: Used by sql_yacc */
> + #define COL_SERIAL_VAL	1<<19		/* Intern: Used by sql_yacc */
> + #define COL_HAS_COMMENT	1<<20		/* Intern: Used by sql_yacc */
>
>   #define REFRESH_GRANT		1	/* Refresh grant tables */
>   #define REFRESH_LOG		2	/* Start on new log file */
>   #define REFRESH_TABLES		4	/* close all tables */

Okay.

Thank you very much.  Very few problems, all minor.

Of your second test file attached to the bug report, "2", it appears  
to be generated.  How sure are you that the  results are correct?  Is  
there some general principle that could be applied as a comment?  Or  
better, some assertion (via "regex_replace" perhaps?) from the test  
file that "closes the loop" to show correctness?

- chad

--
Chad Miller, Software Developer                         chad@stripped
MySQL Inc., www.mysql.com
Orlando, Florida, USA                                13-20z,  UTC-0400
Office: [phone]                         sip:6740@stripped



Attachment: [application/pgp-signature] This is a digitally signed message part PGP.sig