← Back to team overview

maria-developers team mailing list archive

Re: core review #1

 

Hi Shubham,

On Sun, Jun 5, 2016 at 9:25 AM, Sergei Golubchik <serg@xxxxxxxxxxx> wrote:

> 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.
>

You can refer to the link below for coding guidelines.

https://dev.mysql.com/doc/internals/en/general-development-guidelines.html

Also, if you use vim, there are some good vim suggestions to fix
indentations.

Best,
Nirbhay


>
> 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
>
> _______________________________________________
> Mailing list: https://launchpad.net/~maria-developers
> Post to     : maria-developers@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~maria-developers
> More help   : https://help.launchpad.net/ListHelp
>

References