maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Shubham!

Here's a first review.

Summary: looks good so far, main comments:

1. Coding style. Use the same coding style as everywhere else in the
   file you're editing. It's the same in sql/ and in myisam/ directories.
   But InnoDB uses different coding style.

2. Tests, tests, tests! Please start adding tests now, and commit them
   into your branch. They should be somewhere under mysql-test/ directory.
   The full documentation is here: https://mariadb.com/kb/en/mariadb/mysqltest/
   But even without it you can create your tests by starting of from
   existing test files.

More comments below:

> diff --git a/sql/sql_table.cc b/sql/sql_table.cc
> index dfce503..31f282b 100644
> --- a/sql/sql_table.cc
> +++ b/sql/sql_table.cc
> @@ -3876,8 +3876,9 @@ mysql_prepare_create_table(THD *thd, HA_CREATE_INFO *create_info,
>              column->length= MAX_LEN_GEOM_POINT_FIELD;
>  	  if (!column->length)
>  	  {
> -	    my_error(ER_BLOB_KEY_WITHOUT_LENGTH, MYF(0), column->field_name.str);
> -	    DBUG_RETURN(TRUE);
> +	  	key_info->algorithm=HA_KEY_ALG_HASH;

There's more to it. The task title is, indeed, "unique index for
blobs", but the actual goal is to have unique indexes for anything
that is too long for normal btree indexes. That's what MI_UNIQUE does.

so, you should support unique indexes also for blobs where column->length
is specified, but is too large. Or unique indexes for a combination of
ten long varchar columns, for example.

So, you also need to patch the code where it issues ER_TOO_LONG_KEY.

Also we'd want to use MI_UNIQUE for keys where a user has
explicitly specified USING HASH, but let's look at it later

> +	  //  my_error(ER_BLOB_KEY_WITHOUT_LENGTH, MYF(0), column->field_name.str);
> +	  //  DBUG_RETURN(TRUE);
>  	  }
>  	}
>  #ifdef HAVE_SPATIAL
> diff --git a/storage/myisam/ha_myisam.cc b/storage/myisam/ha_myisam.cc
> index 72bc4c0..9aba216 100644
> --- a/storage/myisam/ha_myisam.cc
> +++ b/storage/myisam/ha_myisam.cc
> @@ -216,44 +216,59 @@ static void mi_check_print_msg(HA_CHECK *param,	const char* msg_type,
>      0  OK
>      !0 error code
>  */
> -
>  int table2myisam(TABLE *table_arg, MI_KEYDEF **keydef_out,
> -                 MI_COLUMNDEF **recinfo_out, uint *records_out)
> +                 MI_COLUMNDEF **recinfo_out,MI_UNIQUEDEF **uniquedef_out ,uint *records_out)
>  {
> -  uint i, j, recpos, minpos, fieldpos, temp_length, length;
> +  uint i, j, recpos, minpos, fieldpos, temp_length, length, k, l, m;
>    enum ha_base_keytype type= HA_KEYTYPE_BINARY;
>    uchar *record;
>    KEY *pos;
>    MI_KEYDEF *keydef;
> +  MI_UNIQUEDEF *uniquedef;
>    MI_COLUMNDEF *recinfo, *recinfo_pos;
>    HA_KEYSEG *keyseg;
>    TABLE_SHARE *share= table_arg->s;
>    uint options= share->db_options_in_use;
>    DBUG_ENTER("table2myisam");
> +  pos= table_arg->key_info;
> +  share->uniques=0;
> +  for (i= 0; i < share->keys; i++,pos++)
> +  {
> +        if(pos->algorithm==HA_KEY_ALG_HASH)
> +        {
> +        	share->uniques++ ;
> +        }
> +  }
>    if (!(my_multi_malloc(MYF(MY_WME),
>            recinfo_out, (share->fields * 2 + 2) * sizeof(MI_COLUMNDEF),
> -          keydef_out, share->keys * sizeof(MI_KEYDEF),
> +          keydef_out, (share->keys - share->uniques) * sizeof(MI_KEYDEF),
> +          uniquedef_out, share->uniques * sizeof(MI_UNIQUEDEF),
>            &keyseg,
>            (share->key_parts + share->keys) * sizeof(HA_KEYSEG),
>            NullS)))
>      DBUG_RETURN(HA_ERR_OUT_OF_MEM); /* purecov: inspected */
>    keydef= *keydef_out;
>    recinfo= *recinfo_out;
> +  uniquedef= *uniquedef_out;
>     pos= table_arg->key_info;
> +   k=0;
> +   m=0;

better call your indexes 'k' and 'u', for keydefs and uniques.

>    for (i= 0; i < share->keys; i++, pos++)
>    {
> -    keydef[i].flag= ((uint16) pos->flags & (HA_NOSAME | HA_FULLTEXT | HA_SPATIAL));
> -    keydef[i].key_alg= pos->algorithm == HA_KEY_ALG_UNDEF ?
> +    if(pos->algorithm!=HA_KEY_ALG_HASH)
> +    {
> +    keydef[k].flag= ((uint16) pos->flags & (HA_NOSAME | HA_FULLTEXT | HA_SPATIAL));
> +    keydef[k].key_alg= pos->algorithm == HA_KEY_ALG_UNDEF ?
>        (pos->flags & HA_SPATIAL ? HA_KEY_ALG_RTREE : HA_KEY_ALG_BTREE) :
>        pos->algorithm;
> -    keydef[i].block_length= pos->block_size;
> -    keydef[i].seg= keyseg;
> -    keydef[i].keysegs= pos->user_defined_key_parts;
> +    keydef[k].block_length= pos->block_size;
> +    keydef[k].seg= keyseg;
> +    keydef[k].keysegs= pos->user_defined_key_parts;
>      for (j= 0; j < pos->user_defined_key_parts; j++)
>      {
>        Field *field= pos->key_part[j].field;
>        type= field->key_type();
> -      keydef[i].seg[j].flag= pos->key_part[j].key_part_flag;
> +      keydef[k].seg[j].flag= pos->key_part[j].key_part_flag;
>  
>        if (options & HA_OPTION_PACK_KEYS ||
>            (pos->flags & (HA_PACK_KEY | HA_BINARY_PACK_KEY |
> @@ -266,52 +281,104 @@ int table2myisam(TABLE *table_arg, MI_KEYDEF **keydef_out,
>          {
>            /* No blobs here */
>            if (j == 0)
> -            keydef[i].flag|= HA_PACK_KEY;
> +            keydef[k].flag|= HA_PACK_KEY;
>            if (!(field->flags & ZEROFILL_FLAG) &&
>                (field->type() == MYSQL_TYPE_STRING ||
>                 field->type() == MYSQL_TYPE_VAR_STRING ||
>                 ((int) (pos->key_part[j].length - field->decimals())) >= 4))
> -            keydef[i].seg[j].flag|= HA_SPACE_PACK;
> +            keydef[k].seg[j].flag|= HA_SPACE_PACK;
>          }
>          else if (j == 0 && (!(pos->flags & HA_NOSAME) || pos->key_length > 16))
> -          keydef[i].flag|= HA_BINARY_PACK_KEY;
> +          keydef[k].flag|= HA_BINARY_PACK_KEY;
>        }
> -      keydef[i].seg[j].type= (int) type;
> -      keydef[i].seg[j].start= pos->key_part[j].offset;
> -      keydef[i].seg[j].length= pos->key_part[j].length;
> -      keydef[i].seg[j].bit_start= keydef[i].seg[j].bit_end=
> -        keydef[i].seg[j].bit_length= 0;
> -      keydef[i].seg[j].bit_pos= 0;
> -      keydef[i].seg[j].language= field->charset_for_protocol()->number;
> +      keydef[k].seg[j].type= (int) type;
> +      keydef[k].seg[j].start= pos->key_part[j].offset;
> +      keydef[k].seg[j].length= pos->key_part[j].length;
> +      keydef[k].seg[j].bit_start= keydef[k].seg[j].bit_end=
> +        keydef[k].seg[j].bit_length= 0;
> +      keydef[k].seg[j].bit_pos= 0;
> +      keydef[k].seg[j].language= field->charset_for_protocol()->number;
>  
>        if (field->null_ptr)
>        {
> -        keydef[i].seg[j].null_bit= field->null_bit;
> -        keydef[i].seg[j].null_pos= (uint) (field->null_ptr-
> +        keydef[k].seg[j].null_bit= field->null_bit;
> +        keydef[k].seg[j].null_pos= (uint) (field->null_ptr-
>                                             (uchar*) table_arg->record[0]);
>        }
>        else
>        {
> -        keydef[i].seg[j].null_bit= 0;
> -        keydef[i].seg[j].null_pos= 0;
> +        keydef[k].seg[j].null_bit= 0;
> +        keydef[k].seg[j].null_pos= 0;
>        }
>        if (field->type() == MYSQL_TYPE_BLOB ||
>            field->type() == MYSQL_TYPE_GEOMETRY)
>        {
> -        keydef[i].seg[j].flag|= HA_BLOB_PART;
> +        keydef[k].seg[j].flag|= HA_BLOB_PART;
>          /* save number of bytes used to pack length */
> -        keydef[i].seg[j].bit_start= (uint) (field->pack_length() -
> +        keydef[k].seg[j].bit_start= (uint) (field->pack_length() -
>                                              portable_sizeof_char_ptr);
>        }
>        else if (field->type() == MYSQL_TYPE_BIT)
>        {
> -        keydef[i].seg[j].bit_length= ((Field_bit *) field)->bit_len;
> -        keydef[i].seg[j].bit_start= ((Field_bit *) field)->bit_ofs;
> -        keydef[i].seg[j].bit_pos= (uint) (((Field_bit *) field)->bit_ptr -
> +        keydef[k].seg[j].bit_length= ((Field_bit *) field)->bit_len;
> +        keydef[k].seg[j].bit_start= ((Field_bit *) field)->bit_ofs;
> +        keydef[k].seg[j].bit_pos= (uint) (((Field_bit *) field)->bit_ptr -
>                                            (uchar*) table_arg->record[0]);
>        }
>      }
>      keyseg+= pos->user_defined_key_parts;
> +    k++;
> +    }
> +    else
> +    {
> +      uniquedef[m].sql_key_no=i;
> +    	uniquedef[m].null_are_equal=1;
> +    	 uniquedef[m].seg=keyseg;

please follow code formatting rules that you can see elsewhere in this file

> +    	uniquedef[m].keysegs= pos->user_defined_key_parts;
> +    for (l= 0; l < pos->user_defined_key_parts; l++)
> +    {
> +    	Field *field= pos->key_part[l].field;
> +      type= field->key_type();
> +      uniquedef[m].seg[l].flag= pos->key_part[l].key_part_flag;
> +     uniquedef[m].seg[l].type= (int) type;
> +      uniquedef[m].seg[l].start= pos->key_part[l].offset;
> +     uniquedef[m].seg[l].length= pos->key_part[l].length;
> +      uniquedef[m].seg[l].bit_start= uniquedef[m].seg[l].bit_end=
> +        uniquedef[m].seg[l].bit_length= 0;
> +      uniquedef[m].seg[l].bit_pos= 0;
> +     uniquedef[m].seg[l].language= field->charset_for_protocol()->number;
> +
> +      if (field->null_ptr)
> +      {
> +        uniquedef[m].seg[l].null_bit= field->null_bit;
> +        uniquedef[m].seg[l].null_pos= (uint) (field->null_ptr-
> +                                           (uchar*) table_arg->record[0]);
> +      }
> +      else
> +      {
> +        uniquedef[m].seg[l].null_bit= 0;
> +        uniquedef[m].seg[l].null_pos= 0;
> +      }
> +      if (field->type() == MYSQL_TYPE_BLOB ||
> +          field->type() == MYSQL_TYPE_GEOMETRY)
> +      {
> +        uniquedef[m].seg[l].flag|= HA_BLOB_PART;
> +        /* save number of bytes used to pack length */
> +        uniquedef[m].seg[l].bit_start= (uint) (field->pack_length() -
> +                                            portable_sizeof_char_ptr);
> +      }
> +      else if (field->type() == MYSQL_TYPE_BIT)
> +      {
> +        uniquedef[m].seg[l].bit_length= ((Field_bit *) field)->bit_len;
> +        uniquedef[m].seg[l].bit_start= ((Field_bit *) field)->bit_ofs;
> +        uniquedef[m].seg[l].bit_pos= (uint) (((Field_bit *) field)->bit_ptr -
> +                                          (uchar*) table_arg->record[0]);
> +      }

this seems to be the same code as for keydefs, isn't it?
better to extract it into a separate function. Like:

   setup_keyparts(pos, keydef[k].seg);
   setup_keyparts(pos, uniquedef[m].seg);

> +
> +    }
> +    keyseg+= pos->user_defined_key_parts;
> +    m++;
> +    }
>    }
>    if (table_arg->found_next_number_field)
>      keydef[share->next_number_index].flag|= HA_AUTO_KEY;
> @@ -453,6 +528,7 @@ int check_definition(MI_KEYDEF *t1_keyinfo, MI_COLUMNDEF *t1_recinfo,
>                       MI_KEYDEF *t2_keyinfo, MI_COLUMNDEF *t2_recinfo,
>                       uint t2_keys, uint t2_recs, bool strict, TABLE *table_arg)
>  {
> +	return 0;

to be fixed, I suppose?

>    uint i, j;
>    DBUG_ENTER("check_definition");
>    my_bool mysql_40_compat= table_arg && table_arg->s->frm_version < FRM_VER_TRUE_VARCHAR;
> diff --git a/storage/myisam/mi_write.c b/storage/myisam/mi_write.c
> index ff96ee8..4d2b62a 100644
> --- a/storage/myisam/mi_write.c
> +++ b/storage/myisam/mi_write.c
> @@ -188,6 +189,28 @@ int mi_write(MI_INFO *info, uchar *record)
>          mi_flush_bulk_insert(info, j);
>      }
>      info->errkey= (int) i;
> +    prev=-1;
> +    keydef_no=0;
> +  for (k=0 ; k < share->state.header.uniques ; k++)
> +  {
> +    MI_UNIQUEDEF *def= share->uniqueinfo + k;
> +
> +    if(keydef_no < (int) i+1)
> +    {
> +       diff= ((int)def->sql_key_no) - prev -1;
> +       keydef_no += diff;
> +    }
> +    prev= (int) def->sql_key_no;
> +    if(keydef_no <= (int) i+1)
> +    {
> +        info->errkey += 1;
> +    }  
> +    else
> +      break;

this can be done a bit simpler, without diff, prev, or keydef_no:

 for (k=0; k < share->state.header.uniques; k++, i++)
   if (i < share->uniqueinfo[k].sql_key_no)
     break;
 info->errkey= (int) i;

> +    
> +  }
> +
> +   
>      while ( i-- > 0)
>      {
>        if (mi_is_key_active(share->state.key_map, i))

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx