maria-developers team mailing list archive

[Sachin Setiya <sachin.setiya@xxxxxxxxxxx>]

Hi Serg!

Thanks for the review.
On Fri, Nov 24, 2017 at 3:06 AM, Sergei Golubchik <serg@xxxxxxxxxxx> wrote:
> 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 :)
>
Sorry, Removed.
>>       /* 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))
>
Done.
>>
>>  #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
>
Done.
>>    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
>
> 1. Hmm, you will create a new Item for every inserted row?
> What if it's INSERT ... SELECT * FROM very_large_table?
>
> 2. please test inserts with stored procedures and prepared statements.
>
Done.
>> +    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 :)
>
Ohh sorry.
Removed all instance of hidden in code.
>>    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"
>
Changed :)
>> +  }
>> +  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
>
Sorry this was changed in next commit.
>>  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"
>
Done
>> +*/
>> +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
>
Done.
>> +
>> +  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"
>
Okay , I just thought 1000 will be more then enough.
changed.

>> +  {
>> +    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?
Right
> 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?
Changed this , Now they are deleted and recreated again.
>> +          */
>> +          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



-- 
Regards
Sachin Setiya
Software Engineer at  MariaDB