maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Sachin!

On Nov 22, Sachin Setiya wrote:
> Hi Serg!
> 
> Thanks for the review.
> Please review the updated patch , at bb-hidden-test branch
> I am currently working on this , only 2 things are left.
> Changes apart from comments.
> Rebased to latest 10.3.
> Completely_invisible, or HA_INVISIBLE_INDEX is only deleted when same
> name column/
> index is created(with tests).

Thanks! I've only looked at diffs from my last review this time.
Few minor comments below:

> diff --git a/include/my_base.h b/include/my_base.h
> index b93300d7562..e028922b220 100644
> --- a/include/my_base.h
> +++ b/include/my_base.h
> @@ -277,7 +277,8 @@ enum ha_base_keytype {
>    This flag can be calculated -- it's based on key lengths comparison.
>  */
>  #define HA_KEY_HAS_PART_KEY_SEG 65536
> -
> +/* Internal Flag Can be calcaluted */
> +#define HA_INVISIBLE_KEY 2<<18 /* An invisible key */

I'm not a great fan of tautological comments like

   a++; // increment a

ideally, the code is self-documenting and needs no comments.
sometimes comments are needed, mostly to explain *why* something
is done, not *what* is done.

in this case HA_INVISIBLE_KEY is very clear on itself,
no need to repeat it in a comment :)

>  	/* Automatic bits in key-flag */
>  
>  #define HA_SPACE_PACK_USED	 4	/* Test for if SPACE_PACK used */
> diff --git a/sql/field.h b/sql/field.h
> index 139b292c96a..ac522f2cf16 100644
> --- a/sql/field.h
> +++ b/sql/field.h
> @@ -4477,5 +4480,6 @@ bool check_expression(Virtual_column_info *vcol, LEX_CSTRING *name,
>  #define f_no_default(x)		((x) & FIELDFLAG_NO_DEFAULT)
>  #define f_bit_as_char(x)        ((x) & FIELDFLAG_TREAT_BIT_AS_CHAR)
>  #define f_is_hex_escape(x)      ((x) & FIELDFLAG_HEX_ESCAPE)
> +#define f_visibility(x)         (static_cast<field_visible_type> (x & 3))

note, it must be (x) - always in parentheses
think of

    f_visibility(flags | 5)

you don't want to expand it to

    (static_cast<field_visible_type> (flags | 5 & 3))

>  
>  #endif /* FIELD_INCLUDED */
> diff --git a/sql/sql_base.cc b/sql/sql_base.cc
> index 93dd6239749..f8114b429db 100644
> --- a/sql/sql_base.cc
> +++ b/sql/sql_base.cc
> @@ -8153,13 +8166,19 @@ fill_record(THD *thd, TABLE *table, Field **ptr, List<Item> &values,
>    List<TABLE> tbl_list;
>    bool all_fields_have_values= true;
>    Item *value;
> -  Field *field;
> +  Field *field, **f;
>    bool abort_on_warning_saved= thd->abort_on_warning;
>    uint autoinc_index= table->next_number_field
>                          ? table->next_number_field->field_index
>                          : ~0U;
> +  uint field_count= 0;
> +  bool need_default_value= false;
>    DBUG_ENTER("fill_record");
> -
> +  //TODO will fields count be alwats equal to table->fields ?
> +  for (f= ptr; f && (field= *f); f++)
> +    field_count++;
> +  if (field_count != values.elements)
> +    need_default_value= true;

TODO from the previous review

>    if (!*ptr)
>    {
>      /* No fields to update, quite strange!*/
> @@ -8177,12 +8196,16 @@ fill_record(THD *thd, TABLE *table, Field **ptr, List<Item> &values,
>      only one row.
>    */
>    table->auto_increment_field_not_null= FALSE;
> +  Name_resolution_context *context= & thd->lex->select_lex.context;
>    while ((field = *ptr++) && ! thd->is_error())
>    {
>      /* Ensure that all fields are from the same table */
>      DBUG_ASSERT(field->table == table);
>  
> -    value=v++;
> +    if (need_default_value && field->field_visibility != NOT_INVISIBLE)
> +      value = new (thd->mem_root) Item_default_value(thd,context);

TODO from the previous review

> +    else
> +      value=v++;
>      if (field->field_index == autoinc_index)
>        table->auto_increment_field_not_null= TRUE;
>      if (field->vcol_info)
> diff --git a/sql/sql_class.h b/sql/sql_class.h
> index 92f28a4dc07..87fbb56a557 100644
> --- a/sql/sql_class.h
> +++ b/sql/sql_class.h
> @@ -790,6 +793,7 @@ typedef struct system_status_var
>    ulong feature_dynamic_columns;    /* +1 when creating a dynamic column */
>    ulong feature_fulltext;	    /* +1 when MATCH is used */
>    ulong feature_gis;                /* +1 opening a table with GIS features */
> +  ulong feature_invisible_columns;     /* +1 opening a table with hidden column */

another "hidden" missed - in a comment :)

>    ulong feature_locale;		    /* +1 when LOCALE is set */
>    ulong feature_subquery;	    /* +1 when subqueries are used */
>    ulong feature_timezone;	    /* +1 when XPATH is used */
> diff --git a/sql/sql_table.cc b/sql/sql_table.cc
> index 4006e3aec4d..5cfdaddaa88 100644
> --- a/sql/sql_table.cc
> +++ b/sql/sql_table.cc
> @@ -3232,8 +3236,57 @@ bool Column_definition::prepare_stage1_check_typelib_default()
>    }
>    return false;
>  }
> -
> -
> +/*
> +   This function adds a hidden field to field_list
> +   SYNOPSIS
> +    mysql_add_hidden_field()
> +      thd                      Thread Object
> +      field_list               list of all table fields
> +      field_name               name/prefix of hidden field
> +                               ( Prefix in the case when it is
> +                                *COMPLETELY_INVISIBLE*
> +                               and given name is duplicate)
> +      type_handler             field data type
> +      field_visibility
> +      default value
> +    RETURN VALUE
> +      Create_field pointer
> +*/
> +int mysql_add_invisible_field(THD *thd, List<Create_field> * field_list,
> +        const char *field_name, Type_handler *type_handler,
> +        field_visible_type field_visibility, Item* default_value)
> +{
> +  Create_field *fld= new(thd->mem_root)Create_field();
> +  const char *new_name= NULL;
> +  /* Get unique field name if field_visibility == COMPLETELY_INVISIBLE */
> +  if (field_visibility == COMPLETELY_INVISIBLE)
> +  {
> +    if ((new_name= make_unique_invisible_field_name(thd, field_name,
> +                                                     field_list)))
> +    {
> +      fld->field_name.str= new_name;
> +      fld->field_name.length= strlen(new_name);
> +    }
> +    else
> +      return 1;  //Should not never happen

are you a bit too negative here? :)
"should not happen" or "should never happen"

> +  }
> +  else
> +  {
> +    fld->field_name.str= thd->strmake(field_name, strlen(field_name));
> +    fld->field_name.length= strlen(field_name);
> +  }
> +  fld->set_handler(type_handler);
> +  fld->field_visibility= field_visibility;
> +  if (default_value)
> +  {
> +    Virtual_column_info *v= new (thd->mem_root) Virtual_column_info();
> +    v->expr= default_value;
> +    v->utf8= 0;
> +    fld->default_value= v;
> +  }
> +  field_list->push_front(fld, thd->mem_root);
> +  return 0;
> +}
>  /*
>    Preparation for table creation
>  
> @@ -5001,17 +5098,36 @@ bool mysql_create_table(THD *thd, TABLE_LIST *create_table,
>  
>  /*
>  ** Give the key name after the first field with an optional '_#' after
> +   @returns
> +    0        if keyname does not exists
> +    [1..)    index + 1 of duplicate key name
>  **/
>  
>  static bool

if you want the function to return the key number,
you need to change the return type

>  check_if_keyname_exists(const char *name, KEY *start, KEY *end)
>  {
> -  for (KEY *key=start ; key != end ; key++)
> +  uint i= 1;
> +  for (KEY *key=start; key != end ; key++, i++)
>      if (!my_strcasecmp(system_charset_info, name, key->name.str))
> -      return 1;
> +      return i;
>    return 0;
>  }
>  
> +/**
> + Returns 1 if field name exists other wise 0

"otherwise"

> +*/
> +static bool
> +check_if_field_name_exists(const char *name, List<Create_field> * fields)
> +{
> +  Create_field *fld;
> +  List_iterator<Create_field>it(*fields);
> +  while ((fld = it++))
> +  {
> +    if (!my_strcasecmp(system_charset_info, fld->field_name.str, name))
> +      return 1;
> +  }
> +  return 0;
> +}
>  
>  static char *
>  make_unique_key_name(THD *thd, const char *field_name,KEY *start,KEY *end)
> @@ -5069,6 +5185,31 @@ static void make_unique_constraint_name(THD *thd, LEX_CSTRING *name,
>    }
>  }
>  
> +/**
> +  COMPLETELY_INVISIBLE are internally created. They are completely invisible
> +  to Alter command (Opposite of SYSTEM_INVISIBLE which through
> +  error when same name column is added by Alter). So in the case of when
> +  user added a same column name as of COMPLETELY_INVISIBLE , we change
> +  COMPLETELY_INVISIBLE column name.
> +*/
> +static const
> +char * make_unique_invisible_field_name(THD *thd, const char *field_name,
> +                        List<Create_field> *fields)
> +{
> +  if (!check_if_field_name_exists(field_name, fields))
> +    return field_name;
> +  char buff[MAX_FIELD_NAME], *buff_end;
> +  buff_end= strmov(buff, field_name);

You never check that buff is long enough. Use something like

   buff_end= strnmov(buff, field_name, MAX_FIELD_NAME);
   if (buff_end - buff < 5)
      return NULL; // Should not happen

> +
> +  for (uint i=1 ; i < 1000; i++)

make it 10000, one can still get 4000-5000 fields in a table,
but not more, so after 10000 you can safely say "should not happen"

> +  {
> +    char *real_end= int10_to_str(i, buff_end, 10);
> +    if (check_if_field_name_exists(buff, fields))
> +      continue;
> +    return (const char *)thd->strmake(buff, real_end - buff);
> +  }
> +  return NULL; //Should not happen
> +}
>  
>  /****************************************************************************
>  ** Alter a table definition
> @@ -7900,6 +8045,30 @@ mysql_prepare_alter_table(THD *thd, TABLE *table,
>  	my_error(ER_WRONG_NAME_FOR_INDEX, MYF(0), key->name.str);
>          goto err;
>        }
> +      uint dup_index= 0;
> +      if ((dup_index= check_if_keyname_exists(key->name.str, table->key_info,
> +                    table->key_info + table->s->keys)))
> +      {
> +        if (table->s->key_info[dup_index - 1].flags & HA_INVISIBLE_KEY)
> +        {
> +          /* Drop Index from new_key_list
> +             Why only drop HA_INVISIBLE_INDEX because we want them to be renamed
> +               automatically.

1. so, you don't drop invisible indexes if there is no name conflict?
2. where are they recreated?
3. the recreation code needs to check whether the index is present
   and recreate if not. why not to recreate always? and drop always?
   isn't that simpler?

> +          */
> +          List_iterator<Key> it(new_key_list);
> +          Key *tmp;
> +          while((tmp= it++))
> +          {
> +            if(!my_strcasecmp(system_charset_info, tmp->name.str, key->name.str))
> +            {
> +              it.remove();
> +              if (table->s->tmp_table == NO_TMP_TABLE)
> +                (void) delete_statistics_for_index(thd, table,
> +                         &table->key_info[dup_index - 1], FALSE);
> +            }
> +          }
> +        }
> +      }
>      }
>    }
>  

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx