maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Sachin!

On Jun 06, Sachin Setia wrote:
> Hello Sir
> Weekly Report for second week of gsoc
> 1. Implemented unique in the case of null
> 2. Implemented unique(A,b,c....)
> 
> Currently working on
> tests , field hiding

Good, thanks!

I'd suggest to postpone field hiding until bb-10.2-default
branch is merged into 10.2.

Start with tests, it's very important to have them.
See mysql-test/t/*.test and the manual is here:
https://mariadb.com/kb/en/mariadb/mysql-test/
Don't forget direct tests of your HASH function.

Then, may be, look at UPDATE.

And here's a code review up to the commit b69e141

> diff --git a/sql/handler.cc b/sql/handler.cc
> index fb2921f..e68e37b 100644
> --- a/sql/handler.cc
> +++ b/sql/handler.cc
> @@ -5863,16 +5863,86 @@ int handler::ha_reset()
>    DBUG_RETURN(reset());
>  }
>  
> +/**  @breif

typo: brief

> +   Compare two records
> +   Need a better place for this function 
> +   @returns true if equal else false*/
> +my_bool rec_hash_cmp(TABLE *tbl ,Field * hash_field)

in C++ you can use 'bool'. 'my_bool' is only for C

and, please, pay attention to where you put spaces :)

> +{
> +  Item * t_item;
> +  t_item=hash_field->vcol_info->expr_item->next;

No, please don't rely on the 'next' pointer. It used to link
all items into a list, so that they could be destroyed all at once.
It's not how you traverse expression arguments.

Use, for example,

   Item **arg,**arg_end;
   for (arg=args, arg_end=args+arg_count; arg != arg_end ; arg++)
   {
     Item t_item= *arg;

or
 for (uint i= 0; i < arg_count; i++)
 {
   Item *t_item= args[i];

See these and other examples in item_func.cc

> +  int diff = tbl->record[1]-tbl->record[0];

record[1] - record[0] is always table->s->rec_buff_length
But there's one problem with that - you cannot use record[1],
see a comment below.

> +  Field * t_field;
> +  int first_length,second_length;
> +  while(t_item)
> +  {
> +    for(int i=0;i<tbl->s->fields;i++)
> +    {
> +      t_field=tbl->field[i];
> +      if(!my_strcasecmp(system_charset_info,
> +           t_item->name,t_field->field_name))
> +        break;
> +    }

your t_item is an Item_field, so your t_field
will be t_item->field, no need to iterate anything.

> +    /* First check for nulls */
> +    if(t_field->is_real_null()||t_field->is_real_null(diff))
> +    {
> +      return false; 
> +    }

This shouldn't be necessary, see below, you shouldn't call this function
when a field value is NULL.

>  
> +    /*
> +       length of field is not equal to pack length when it is
> +       subclass of field_blob and field _varsting
> +     */
> +    first_length = t_field->data_length();
> +    second_length = t_field->data_length(diff);
> +    if(first_length!=second_length)
> +      return false;
> +    if(t_field->cmp_max(t_field->ptr,t_field->ptr+diff,first_length))
> +      return false;
> +    t_item=t_item->next;
> +  }
> +  return true;
> +}
>  int handler::ha_write_row(uchar *buf)
>  {
> -  int error;
> +  int error,map;
>    Log_func *log_func= Write_rows_log_event::binlog_row_logging_function;
> +  Field **field_iter;
>    DBUG_ASSERT(table_share->tmp_table != NO_TMP_TABLE ||
>                m_lock_type == F_WRLCK);
>    DBUG_ENTER("handler::ha_write_row");
>    DEBUG_SYNC_C("ha_write_row_start");
> +  /* First need to whether inserted record is unique or not */
>  
> +  /* One More Thing  if i implement hidden field then detection can be easy */
> +  field_iter=table->field;
> +  for(uint i=0;i<table->s->fields;i++)

You can iterate only virtual fields, there's a separate list for them.

> +  {
> +    if(strncmp((*field_iter)->field_name,"DB_ROW_HASH_",12)==0)
> +    {
> +      table->file->ha_index_init(0,0);

this might be a problem, as it changes the current index.
I think it's ok for INSERT, but may be not ok for UPDATE
(imagine, UPDATE ... WHERE indexed_column=5, the server does
ha_index_init for indexed_column, and then, in a loop,
ha_index_next/ha_update_row. So you cannot swicth to another index
in the middle of UPDATE loop)

So, use ha_index_read_idx_map(), it doesn't change the current index.

by the way, how could it be ha_index_init(0,0) ???
you need to specify your unique key number, not 0.

> +      /* We need to add the null bit */
> +      /* If the column can be NULL, then in the first byte we put 1 if the
> +	   field value is NULL, 0 otherwise. */
> +      uchar * ptr = (uchar *)my_malloc(sizeof(char ) *1+8, MYF(MY_WME));

1. where do you free it?
2. doing malloc/free for _every row_ is too expensive, mallocs
generally are not very good for concurrency.
You can simply use

   uchar ptr[9]

can you not?

> +      *ptr=0;

why *ptr=0 ? You need to check whether the column value is actually NULL,
because now you set the key to "not null" unconditionally.
Besides, because NULL value can never cause HA_ERR_FOUND_DUPP_KEY,
you don't need to search if the column is NULL, you can skip this whole
check.

> +      ptr++;
> +      memcpy(ptr,(*field_iter)->ptr,8);
> +      ptr--;

No need to go low-level, you can do: Why wouldn't you use
key_copy() function? Like

  uchar buf[9];
  DBUG_ASSERT(key->length == sizeof(buf));
  key_copy(ptr, buf, key, 0, false);

> +      map= table->file->ha_index_read_map(table->record[1],ptr,HA_WHOLE_KEY,HA_READ_KEY_EXACT);

1. why 'map'? what does it mean?
2. Now it's getting a bit tricky. in UPDATE you cannot use record[1] -
it's the row before the update. I'm afraid you'll need to allocate another
table->s->rec_buff_length buffer for this, I don't see any other choice.
Of course, you can do it on demand, on the table's memroot, preserve
between calls, and so on.

> +      if(!map)
> +      {
> +        if(rec_hash_cmp(table,*field_iter))
> +        {
> +          table->file->ha_index_end();
> +          DBUG_RETURN(HA_ERR_FOUND_DUPP_KEY);
> +        }
> +          
> +      }
> +      table->file->ha_index_end();
> +    }
> +    field_iter++;
> +  }
>    MYSQL_INSERT_ROW_START(table_share->db.str, table_share->table_name.str);
>    mark_trx_read_write();
>    increment_statistics(&SSV::ha_write_count);
> diff --git a/sql/item_func.cc b/sql/item_func.cc
> index e5c1f4c..fb23495 100644
> --- a/sql/item_func.cc
> +++ b/sql/item_func.cc
> @@ -1916,6 +1916,41 @@ void Item_func_int_div::fix_length_and_dec()
>  }
>  
>  
> +longlong  Item_func_hash::val_int()
> +{
> +  unsigned_flag =true;

the other way around, no space before, one space after :)

> +  ulong nr1= 1,temp=1, nr2= 0;
> +  CHARSET_INFO *cs;
> +  for(int i=0;i<arg_count;i++)
> +  {
> +    String * str = args[i]->val_str();
> +    if(args[i]->null_value)
> +    {
> +      null_value=1;

don't forget to set null_value=0 if no arguments are NULL

> +      return 0;
> +    }
> +    nr2=args[i]->max_length;

Hmm, I don't know. This makes your hash depend on metadata, not only on data.
I don't think it's a good idea.

> +    cs=str->charset();
> +    char l[4];
> +    int4store(l,str->length());
> +    cs->coll->hash_sort(cs, (uchar *)l,sizeof(str->length()), &temp, &nr2);

not sizeof(str->length()), but sizeof(l).

> +    nr1^=temp;
> +    cs->coll->hash_sort(cs, (uchar *)str->ptr(),str->length(), &temp, &nr2);

Why did you change the hashing function? &nr1 should be the 4th argument
of hash_sort(), no xors afterwards.

If you change it, you need to prove mathematically that your version
is better. That's why I personally prefer to not to mess with it :)

> +    nr1^=temp;
> +  }
> +  return   (longlong)nr1;
> +}
> +
> +
> +void  Item_func_hash::fix_length_and_dec()
> +{
> +  maybe_null= 1;
> +  null_value= 0;

no need to set null_value here, the value is calculated
in val_int, not in fix_length_and_dec.
fix_length_and_dec is called once, at the very beginning of a query,
val_int is called for every row. And you need to set null_value=0
for every row.

> +  decimals= 0;
> +  max_length= 8;
> +}
> +
> +
>  longlong Item_func_mod::int_op()
>  {
>    DBUG_ASSERT(fixed == 1);
> diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc
> index 9b4238b..2c90f3c 100644
> --- a/sql/sql_insert.cc
> +++ b/sql/sql_insert.cc
> @@ -724,6 +723,20 @@ bool mysql_insert(THD *thd,TABLE_LIST *table_list,
>        DBUG_RETURN(TRUE);
>    }
>      

Please mark places you're going to fix later, e.g.
// TODO when we'll have hidden fields, this will be removed

> +  context= &thd->lex->select_lex.context;
> +  Field ** f=table_list->table->field;
> +  List<Item> * i_list = (List<Item> *)values_list.first_node()->info;
> +  for(uint i=0;i<table_list->table->s->fields;i++)
> +  {
> +    if(strncmp((*f)->field_name,"DB_ROW_HASH_",12)==0)
> +    {
> +      i_list->push_back(new (thd->mem_root) 
> +      Item_default_value(thd,context),thd->mem_root);
> +    }
> +    f++;
> +  }
> +  List_iterator_fast<List_item> its(values_list);
> +     
>    lock_type= table_list->lock_type;
>  
>    THD_STAGE_INFO(thd, stage_init);
> diff --git a/sql/sql_table.cc b/sql/sql_table.cc
> index dfce503..ac2988f 100644
> --- a/sql/sql_table.cc
> +++ b/sql/sql_table.cc
> @@ -3012,8 +3012,8 @@ int prepare_create_field(Create_field *sql_field,
>                            (sql_field->decimals << FIELDFLAG_DEC_SHIFT));
>      break;
>    }
> -  if (!(sql_field->flags & NOT_NULL_FLAG) ||
> -      (sql_field->vcol_info))  /* Make virtual columns allow NULL values */
> +  if (!(sql_field->flags & NOT_NULL_FLAG) )
> +     /* (sql_field->vcol_info))   Make virtual columns allow NULL values */

don't forget to test (see above about mysql-test) that your change
does not affect normal generated columns and they are still always nullable.
to test that, create a table with a persistent generated column
with an expression that can return NULL.

>      sql_field->pack_flag|= FIELDFLAG_MAYBE_NULL;
>    if (sql_field->flags & NO_DEFAULT_VALUE_FLAG)
>      sql_field->pack_flag|= FIELDFLAG_NO_DEFAULT;
> @@ -3223,6 +3223,80 @@ mysql_prepare_create_table(THD *thd, HA_CREATE_INFO *create_info,
>    bool tmp_table= create_table_mode == C_ALTER_TABLE;
>    DBUG_ENTER("mysql_prepare_create_table");
>  
> +  /* 
> +    scan the the whole alter list  
> +    and add one field if length of blob is zero 
> +  */

you need a TODO marker here (TODO change to work for all too-long unique keys)

> +    
> +    List_iterator<Key> key_iter(alter_info->key_list);
> +    Key *key_iter_key;
> +    Key_part_spec *temp_colms;
> +    char num = '1';
> diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy
> index 24dc3c4..0135d9c 100644
> --- a/sql/sql_yacc.yy
> +++ b/sql/sql_yacc.yy
> @@ -9543,6 +9543,15 @@ function_call_keyword:
>              if ($$ == NULL)
>                MYSQL_YYABORT;
>            }
> +        /*Currently hash for just 1 field
> +         * */

your comment is already obsolete :)

> +        |HASH_SYM '(' expr_list ')'
> +            {
> +                $$= new (thd->mem_root)Item_func_hash(thd,*$3);
> +                if($$==NULL)
> +                    MYSQL_YYABORT;
> +            }
> +        
>          | INSERT '(' expr ',' expr ',' expr ',' expr ')'
>            {
>              $$= new (thd->mem_root) Item_func_insert(thd, $3, $5, $7, $9);

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx