maria-developers team mailing list archive
-
maria-developers team
-
Mailing list archive
-
Message #09693
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
