Hi Shubham,
On Sun, Jun 5, 2016 at 9:25 AM, Sergei Golubchik
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@mariadb.org
_______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp