[Maria-developers] Review of last part of discover code
Hi! Here is finally the review.
=== modified file 'sql/sql_truncate.cc' --- sql/sql_truncate.cc 2013-01-25 19:40:42 +0000 +++ sql/sql_truncate.cc 2013-02-27 11:23:42 +0000 @@ -350,9 +342,15 @@ bool Truncate_statement::lock_table(THD MYSQL_OPEN_SKIP_TEMPORARY)) DBUG_RETURN(TRUE);
- if (dd_check_storage_engine_flag(thd, table_ref->db, table_ref->table_name, - HTON_CAN_RECREATE, hton_can_recreate)) + handlerton *hton; + if (!ha_table_exists(thd, table_ref->db, table_ref->table_name, &hton) || + !hton || hton == view_pseudo_hton) + { + my_error(ER_NO_SUCH_TABLE, MYF(0), table_ref->db, table_ref->table_name); DBUG_RETURN(TRUE); + } + + *hton_can_recreate= hton->flags & HTON_CAN_RECREATE; }
ha_table_exists really needs a comment about what it can return and what the different values of hton means.
=== modified file 'sql/table.cc' --- sql/table.cc 2013-01-29 14:10:47 +0000 @@ -292,8 +286,8 @@ TABLE_CATEGORY get_table_category(const # Share */
-TABLE_SHARE *alloc_table_share(TABLE_LIST *table_list, char *key, - uint key_length) +TABLE_SHARE *alloc_table_share(const char *db, const char *table_name, + char *key, uint key_length)
Just so you know, the reason that alloc_table_share took a table list as argument was that it would enable us: - We should be able to use LEX_STRING for db and table_name - Would be trivial to add schema if we wanted to later We need to at some point change to use at least LEX_STRING as this will make build_table_filename faster. <cut>
+ if (memcmp(head, STRING_WITH_LEN("TYPE=VIEW\n")) == 0) { - if (head[2] == FRM_VER || head[2] == FRM_VER+1 || - (head[2] >= FRM_VER+3 && head[2] <= FRM_VER+4)) - { - /* Open view only */ - if (db_flags & OPEN_VIEW_ONLY) - { - error_given= 1; - goto err; - } - table_type= 1; - } - else - { - error= 6; // Unkown .frm version - goto err; - }
In your new code, you accept head[2] == FRM_VER +2, which the above code doesn't do. Don't remember why this is skipped, but if we want to be compatible we should skip this one too... But I think it's safe to ignore this change. <cut>
+ if (my_fstat(file, &stats, MYF(0))) + goto err;
The above I don't like. We already store the max possible length of the frm in frm_header+10. Why not store the exact length there and use this instead of doing fstat()? Would save us a file operation (at least for MariaDB files). It would also ensure that we don't easily break if we find a partial .frm file on disk. <cut>
+ share->init_from_binary_frm_image(thd, false, buf, stats.st_size); + error_given= true;
Add a comment why error_given is set. (I know it's old code, but it looks confusing) <cut>
+int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write, <cut> + while (extra2 < e2end) + { + uchar type= *extra2++; + size_t length= *extra2++; + if (!length) + { + length= uint2korr(extra2); + extra2+=2; + if (length < 256) + goto err; + }
Add a test: if (extra2 + length > e2end) goto err; Please also check in other places that we don't run outside of the image.
+ switch (type) { + case EXTRA2_TABLEDEF_VERSION: + if (tabledef_version.str) // see init_from_sql_statement_string() + { + if (length != tabledef_version.length || + memcmp(extra2, tabledef_version.str, length)) + goto err; + } + else + { + uchar *buf= (uchar*) alloc_root(&mem_root, length); + if (!buf) + goto err;
You could have set tabledef_version.str directly and not use buf.
+ memcpy(buf, extra2, length); + tabledef_version.str= buf; + tabledef_version.length= length; + } + break; + case EXTRA2_ENGINE_TABLEOPTS: + if (options) + goto err; + /* remember but delay parsing until we have read fields and keys */ + options= extra2; + options_len= length; + break; + default: + /* abort frm parsing if it's an unknown but important extra2 value */ + if (type >= 128)
128 should probably be a define.
+ goto err; + } + extra2+= length; + } + if (extra2 > e2end) + goto err;
If you have my earlier test, you don't need this one. Better to do the test early...
+ }
- mysql_file_seek(file,pos,MY_SEEK_SET,MYF(0)); - if (mysql_file_read(file, forminfo,288,MYF(MY_NABP))) + if (!(pos= uint4korr(frm_image + 64 + len))) goto err; - share->frm_version= head[2]; + + forminfo= frm_image + pos;
Check that forminfo + FRM_FORM_LENGTH is inside of image <cut>
/* Read keyinformation */ - key_info_length= (uint) uint2korr(head+28); - mysql_file_seek(file, (ulong) uint2korr(head+6), MY_SEEK_SET, MYF(0)); - if (read_string(file,(uchar**) &disk_buff,key_info_length)) - goto err; /* purecov: inspected */ + disk_buff= frm_image + uint2korr(frm_image+6);
Check that disk_buff + key_info_length is within image.
- if (!(extra_segment_buff= (uchar*) my_malloc(n_length + 1, MYF(MY_WME)))) - goto err; - next_chunk= extra_segment_buff; - if (mysql_file_pread(file, extra_segment_buff, - n_length, record_offset + share->reclength, - MYF(MY_NABP))) - { - goto err; - } + next_chunk= frm_image + record_offset + share->reclength; + buff_end= next_chunk + n_length;
Check that buff_end is is within image. <cut>
- if (mysql_file_pread(file, record, (size_t) share->reclength, - record_offset, MYF(MY_NABP))) - goto err; /* purecov: inspected */ + memcpy(record, frm_image + record_offset, share->reclength);
Check that disk_buff + share->reclength is withing image
- mysql_file_seek(file, pos+288, MY_SEEK_SET, MYF(0)); -#ifdef HAVE_CRYPTED_FRM - if (crypted) - { - crypted->decode((char*) forminfo+256,288-256); - if (sint2korr(forminfo+284) != 0) // Should be 0 - goto err; // Wrong password - } -#endif + disk_buff= frm_image + pos + 288;
Check that disk_buff is withing image <cut>
@@ -1420,7 +1360,6 @@ static int open_binary_frm(THD *thd, TAB geom_type= (Field::geometry_type) strpos[14]; charset= &my_charset_bin; #else - error= 4; // unsupported field type goto err; #endif
Shouldn't we get a better error for this case than just BAD_FRM ?
@@ -1578,10 +1523,8 @@ static int open_binary_frm(THD *thd, TAB (TYPELIB*) 0), share->fieldnames.type_names[i]); if (!reg_field) // Not supported field type - { - error= 4; - goto err; /* purecov: inspected */ - } + goto err; +
Shouldn't we get a better error for this case than just BAD_FRM ?
@@ -1607,20 +1550,8 @@ static int open_binary_frm(THD *thd, TAB if (reg_field->unireg_check == Field::NEXT_NUMBER) share->found_next_number_field= field_ptr;
- if (use_hash) - { - if (my_hash_insert(&share->name_hash, - (uchar*) field_ptr)) - { - /* - Set return code 8 here to indicate that an error has - occurred but that the error message already has been - sent (OOM). - */ - error= 8; - goto err; - } - } + if (use_hash && my_hash_insert(&share->name_hash, (uchar*) field_ptr)) + goto err;
Is this change because my_hash_insert() will give an error for OEM? <cut>
@@ -1950,11 +1877,7 @@ static int open_binary_frm(THD *thd, TAB share->default_values, reg_field, &share->next_number_key_offset, &share->next_number_keypart)) < 0) - { - /* Wrong field definition */ - error= 4; - goto err; - } + goto err; // Wrong field definition else reg_field->flags |= AUTO_INCREMENT_FLAG;
As you are cleaning things up, remove else... <cut>
+ case OPEN_FRM_OK: + DBUG_ASSERT(0); // open_table_error() is never called for this one
Add a break after the assert <cut>
- case 6: - strxmov(buff, share->normalized_path.str, reg_ext, NullS); - my_printf_error(ER_NOT_FORM_FILE, - "Table '%-.64s' was created with a different version " - "of MySQL and cannot be read", - MYF(0), buff); - break;
It would be nice to still keep the above error message. We may need it sooner or later! For example, when we find a required table option type that we don't support! In this case we should also print out the MySQL version that was used to create the table!
- case 8: + case OPEN_FRM_NOT_A_TABLE: + my_error(ER_WRONG_OBJECT, MYF(0), share->db.str, + share->table_name.str, "TABLE"); break; - default: /* Better wrong error than none */ - case 4: + case OPEN_FRM_DISCOVER: + DBUG_ASSERT(0); // open_table_error() is never called for this one
Add a break! (Just in case)
+ case OPEN_FRM_CORRUPTED: strxmov(buff, share->normalized_path.str, reg_ext, NullS); my_error(ER_NOT_FORM_FILE, errortype, buff); break;
<cut>
+void prepare_fileinfo(THD *thd, uint reclength, uchar *fileinfo, + HA_CREATE_INFO *create_info, uint keys, KEY *key_info) +{
Change name to prepare_frm_header
- DBUG_ENTER("create_frm"); - - if (create_info->options & HA_LEX_CREATE_TMP_TABLE) - create_flags|= O_EXCL | O_NOFOLLOW;
I couldn't find where you ensured in writefrm that the above flags was added for temporary tables. We really want to have these flags for temporary files to ensure that we fail if the file already exists! We also don't want anyone to create a symlink in /tmp and use that to overwrite another file!
@@ -3367,7 +3204,7 @@ rename_file_ext(const char * from,const char from_b[FN_REFLEN],to_b[FN_REFLEN]; (void) strxmov(from_b,from,ext,NullS); (void) strxmov(to_b,to,ext,NullS); - return (mysql_file_rename(key_file_frm, from_b, to_b, MYF(MY_WME))); + return (mysql_file_rename(key_file_frm, from_b, to_b, MYF(0)));
Why not give a warning if rename fales? At least, use 'ME_JUST_WARNING'
=== modified file 'sql/table.h' --- sql/table.h 2013-01-29 14:10:47 +0000 +++ sql/table.h 2013-03-06 18:27:13 +0000
+/* Adjust number to next larger disk buffer */ +static inline ulong next_io_size(ulong pos) +{ + return MY_ALIGN(pos, IO_SIZE); +} +
Remove above function. Instead store true max length in frominfo+2
+++ sql/unireg.cc 2013-03-18 15:21:57 +0000
<cut>
+ frm.length= 64; // fileinfo;
Change 64 to FRM_HEADER_LENGTH (and use this in other part of the source)
+ frm.length+= extra2_size + 4; // mariadb extra2 frm segment + + int2store(fileinfo+4, extra2_size);
Add comment: // Size of table version and table options Note that you can't use position 4 for this. Position 4 & 5 is reserved for the number of different forms/screens inside the frm. Using this would make the created table unusable for any old version of MySQL / MariaDB. See get_form_pos() in table.cc in MySQL 5.5 Here is the relevant data in the old frm header Pos Information 4-5 Length of form names ; Always 3 in MySQL 8-9 Number of forms ; Always 1 in MySQL 10-13 Position for next form (if we would create one) 64 Name information (int2korr(head+4) bytes) . 4 byte offset/form from file start to where form starts How to fix this in a portable way (instead of using position 4): Position 10-13 are only set by MySQL but never used. This is ALWAYS aligned on IO_SIZE (Ie, byte 10 is always 0). We can use byte 10 to mark that this is a MariaDB .frm file and then we can use bytes 11-13 for whatever we want.
+ int2store(fileinfo+6, frm.length);
Add comment: // Position to key information
+ frm.length+= key_buff_length; + frm.length+= reclength; // row with default values + frm.length+= create_info->extra_size; + + filepos= frm.length; + frm.length+= 288; // forminfo
Replace 288 in the code with FRM_FORM_LENGTH
+ frm.length+= packed_fields_length(create_fields); + + frm_ptr= (uchar*) my_malloc(frm.length, MYF(MY_WME | MY_ZEROFILL | + MY_THREAD_SPECIFIC)); + if (!frm_ptr) + DBUG_RETURN(frm); + + /* write the extra2 segment */ + pos = frm_ptr + 64;
64 -> FRM_HEADER_LENGTH Please move the line: memcpy(frm_ptr, fileinfo, 64); Before the above line. (Makes it easier to see that header is copied)
+ *pos++ = EXTRA2_TABLEDEF_VERSION;
We probably want to add a comment that MySQL stores '/' here and that EXTRA2_TABLEDEF_VERSION can't never be '/' <cut>
+ DBUG_ASSERT(options_len <= 65535); // FIXME if necessary + int2store(pos + 1, options_len);
Why not just use (so that you can get rid of the comment) int3store(pos + 1, options_len);
+ pos+= 3;
+ } + pos= engine_table_options_frm_image(pos, create_info->option_list, + create_fields, keys, key_info); + } + + int4store(pos, filepos); // end of the extra2 segment + pos+= 4; + + DBUG_ASSERT(pos == frm_ptr + uint2korr(fileinfo+6)); + key_info_length= pack_keys(pos, keys, key_info, data_offset);
maxlength=(uint) next_io_size((ulong) (uint2korr(forminfo)+1000)); int2store(forminfo+2,maxlength);
You can skip the above 2 rows and mark the bytes as free to use.
int4store(fileinfo+10,(ulong) (filepos+maxlength));
Same with the above 4 bytes <cut>
} } if (forminfo[46] == (uchar)255) {
Add comment /* New style MySQL 5.5 table comment */ By the way, this was a *stupid* solution for MySQL 5.5 from MySQL 6.0 What it means is that long comments from MySQL 5.5 are hidden in MySQL below 5.5 A better solution would be to always copy up to TABLE_COMMENT_INLINE_MAXLEN bytes into forminfo+47. (Better to have a cut comment than no comment). Can you please do that change? <cut>
+int rea_create_table(THD *thd, LEX_CUSTRING *frm, + const char *path, const char *db, const char *table_name, + HA_CREATE_INFO *create_info, handler *file) { DBUG_ENTER("rea_create_table");
- char frm_name[FN_REFLEN]; - strxmov(frm_name, path, reg_ext, NullS); - if (mysql_create_frm(thd, frm_name, db, table_name, create_info, - create_fields, keys, key_info, file)) + if (file) + { + // TODO don't write frm for temp tables + if (create_info->tmp_table() && + writefrm(path, db, table_name, 0, frm->str, frm->length)) + goto err_handler;
Can you add a test that if it's a memory table we don't write the frm? (Better to do it for other tmp tables, as we need to delete them from the engine during restart). As far as I can see, we only need one test here, one for the error condition and also when we drop the temporary table. Why not send the 'frm' forwards instead of frm->str and frm->length. These two arguments are passed around for a lot of functions. Would be easier if all functions would just take handler. <cut>
@@ -718,10 +535,8 @@ static bool pack_header(uchar *forminfo, my_snprintf(warn_buff, sizeof(warn_buff), ER(ER_TOO_LONG_FIELD_COMMENT), field->field_name, static_cast<ulong>(COLUMN_COMMENT_MAXLEN)); - /* do not push duplicate warnings */ - if (!check_duplicate_warning(current_thd, warn_buff, strlen(warn_buff))) - push_warning(current_thd, MYSQL_ERROR::WARN_LEVEL_WARN, - ER_TOO_LONG_FIELD_COMMENT, warn_buff); + push_warning(current_thd, MYSQL_ERROR::WARN_LEVEL_WARN, + ER_TOO_LONG_FIELD_COMMENT, warn_buff);
You could clean up the above code at the same time. Now we duplicate a lot of code above. The full code should changed as follows: if ((current_thd->variables.sql_mode & (MODE_STRICT_TRANS_TABLES | MODE_STRICT_ALL_TABLES))) { my_error(ER_TOO_LONG_FIELD_COMMENT, MYF(0), field->field_name, static_cast<ulong>(COLUMN_COMMENT_MAXLEN)); DBUG_RETURN(1); } char warn_buff[MYSQL_ERRMSG_SIZE]; my_snprintf(warn_buff, sizeof(warn_buff), ER(ER_TOO_LONG_FIELD_COMMENT), field->field_name, static_cast<ulong>(COLUMN_COMMENT_MAXLEN)); push_warning(current_thd, MYSQL_ERROR::WARN_LEVEL_WARN, ER_TOO_LONG_FIELD_COMMENT, warn_buff); field->comment.length= tmp_len; -> bool strict= (current_thd->variables.sql_mode & (MODE_STRICT_TRANS_TABLES | MODE_STRICT_ALL_TABLES))); my_error(ER_TOO_LONG_FIELD_COMMENT, MYF(strict ? 0 : ME_JUST_WARNING), field->field_name, static_cast<ulong>(COLUMN_COMMENT_MAXLEN)); if (strict) DBUG_RETURN(1); field->comment.length= tmp_len;
@@ -833,7 +647,7 @@ static bool pack_header(uchar *forminfo, } /* Hack to avoid bugs with small static rows in MySQL */ reclength=max(file->min_record_length(table_options),reclength); - if (info_length+(ulong) create_fields.elements*FCOMP+288+ + if ((ulong) create_fields.elements*FCOMP+288+
288 -> FRM_FORM_LENGTH <cut>
- /* Save fields, fieldnames and intervals */ +static size_t packed_fields_length(List<Create_field> &create_fields) +{ + Create_field *field; + size_t length= 0; + DBUG_ENTER("packed_fields_length");
-static bool pack_fields(File file, List<Create_field> &create_fields, + List_iterator<Create_field> it(create_fields); + uint int_count=0; + while ((field=it++)) + { + if (field->interval_id > int_count) + { + int_count= field->interval_id; + length++; + for (int i=0; field->interval->type_names[i]; i++) + { + length+= field->interval->type_lengths[i]; + length++; + } + length++; + } + if (field->vcol_info) + { + length+= field->vcol_info->expr_str.length + + FRM_VCOL_HEADER_SIZE(field->interval);
Should be: length+= field->vcol_info->expr_str.length + FRM_VCOL_HEADER_SIZE(field->interval_id); Why? This is the code where the byte is written in pack_fields(). (The code you copied was also wrong, see below) if (field->interval_id) *buff++= (uchar) field->interval_id; <cut>
+static bool pack_fields(uchar *buff, List<Create_field> &create_fields,
<cut>
@@ -950,40 +791,29 @@ static bool pack_fields(File file, List< the additional data saved for the virtual field */ buff[12]= cur_vcol_expr_len= field->vcol_info->expr_str.length + - FRM_VCOL_HEADER_SIZE(field->interval!=NULL); - vcol_info_length+= cur_vcol_expr_len + - FRM_VCOL_HEADER_SIZE(field->interval!=NULL); + FRM_VCOL_HEADER_SIZE(field->interval); + vcol_info_length+= cur_vcol_expr_len;
Change field->interval to field->interval_id
=== modified file 'storage/archive/ha_archive.cc' --- storage/archive/ha_archive.cc 2013-01-23 15:24:04 +0000 +++ storage/archive/ha_archive.cc 2013-03-06 20:23:01 +0000 +int archive_discover(handlerton *hton, THD* thd, TABLE_SHARE *share) { DBUG_ENTER("archive_discover"); - DBUG_PRINT("archive_discover", ("db: %s, name: %s", db, name)); + DBUG_PRINT("archive_discover", ("db: %s, name: %s", share->db.str, + share->table_name.str));
When you change DBUG_PRINT, please also fix the syntax to be same as the rest of the code. The above should be: DBUG_PRINT("archive_discover", ("db: '%s' name: '%s'", share->db.str, share->table_name.str)); (No comma, double space, ' around string arguments)
@@ -738,56 +748,40 @@ int ha_archive::create(const char *name, <cut> + /* + Here is where we open up the frm and pass it to archive to store + */ + if (!table_arg->s->read_frm_image(&frm_ptr, &frm_len)) + { + azwrite_frm(&create_stream, frm_ptr, frm_len); + my_free(const_cast<uchar*>(frm_ptr)); + }
Generally it would be better to have a function: table_arg->s->free_frm_image(&frm_ptr) Because the server could be compiled with another memory allocator than the storage engine (DBUG / NO_DBUG).
=== modified file 'storage/federatedx/ha_federatedx.cc' --- storage/federatedx/ha_federatedx.cc 2013-01-23 15:24:04 +0000 +++ storage/federatedx/ha_federatedx.cc 2013-03-10 10:49:49 +0000 @@ -3588,6 +3574,71 @@ int ha_federatedx::rollback(handlerton * DBUG_RETURN(return_val); }
+ +/* + Federated supports assisted discovery, like + CREATE TABLE t1 CONNECTION="mysql://joe:pass@192.168.1.111/federated/t1"; + but not a fully automatic discovery where a table magically appear + on any use (like, on SELECT * from t1). +*/ +int ha_federatedx::discover_assisted(handlerton *hton, THD* thd, + TABLE_SHARE *table_s, HA_CREATE_INFO *info)
<cut>
+ query.copy(rdata[1], rlen[1], cs); + query.append(STRING_WITH_LEN(" CONNECTION=\""), cs); + query.append(table_s->connect_string.str, table_s->connect_string.length, cs); + query.append('"');
Shouldn't we need to quote the string properly? (It may have " inside) Other things: - Please run sql_bench/test-create on your tree to verify that things are now faster (should be as we don't have to open a frm when a table is created). - Test it also for temporary memory tables (assuming you can get rid of the .frm for them) Regards, Monty
Hi, Michael! On Apr 07, Michael Widenius wrote:
Here is finally the review.
Thanks!
=== modified file 'sql/sql_truncate.cc' --- sql/sql_truncate.cc 2013-01-25 19:40:42 +0000 +++ sql/sql_truncate.cc 2013-02-27 11:23:42 +0000 @@ -350,9 +342,15 @@ bool Truncate_statement::lock_table(THD MYSQL_OPEN_SKIP_TEMPORARY)) DBUG_RETURN(TRUE);
- if (dd_check_storage_engine_flag(thd, table_ref->db, table_ref->table_name, - HTON_CAN_RECREATE, hton_can_recreate)) + handlerton *hton; + if (!ha_table_exists(thd, table_ref->db, table_ref->table_name, &hton) || + !hton || hton == view_pseudo_hton) + { + my_error(ER_NO_SUCH_TABLE, MYF(0), table_ref->db, table_ref->table_name); DBUG_RETURN(TRUE); + } + + *hton_can_recreate= hton->flags & HTON_CAN_RECREATE; }
ha_table_exists really needs a comment about what it can return and what the different values of hton means.
Done.
=== modified file 'sql/table.cc' --- sql/table.cc 2013-01-29 14:10:47 +0000 @@ -292,8 +286,8 @@ TABLE_CATEGORY get_table_category(const # Share */
-TABLE_SHARE *alloc_table_share(TABLE_LIST *table_list, char *key, - uint key_length) +TABLE_SHARE *alloc_table_share(const char *db, const char *table_name, + char *key, uint key_length)
We need to at some point change to use at least LEX_STRING as this will make build_table_filename faster.
Agree!
+ if (my_fstat(file, &stats, MYF(0))) + goto err;
The above I don't like.
We already store the max possible length of the frm in frm_header+10. Why not store the exact length there and use this instead of doing fstat()? Would save us a file operation (at least for MariaDB files). It would also ensure that we don't easily break if we find a partial .frm file on disk.
Done. Also added a limit, as we discussed: frmlen= uint4korr(head+10); set_if_smaller(frmlen, FRM_MAX_SIZE); // safety
+ share->init_from_binary_frm_image(thd, false, buf, stats.st_size); + error_given= true;
Add a comment why error_given is set. (I know it's old code, but it looks confusing)
Done.
+int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write, <cut> + while (extra2 < e2end) + { + uchar type= *extra2++; + size_t length= *extra2++; + if (!length) + { + length= uint2korr(extra2); + extra2+=2; + if (length < 256) + goto err; + }
Add a test: if (extra2 + length > e2end) goto err;
Please also check in other places that we don't run outside of the image.
Done.
+ switch (type) { + case EXTRA2_TABLEDEF_VERSION: + if (tabledef_version.str) // see init_from_sql_statement_string() + { + if (length != tabledef_version.length || + memcmp(extra2, tabledef_version.str, length)) + goto err; + } + else + { + uchar *buf= (uchar*) alloc_root(&mem_root, length); + if (!buf) + goto err;
You could have set tabledef_version.str directly and not use buf.
How comes? We need to copy the version into TABLE_SHARE's memroot, to make sure it has the same lifetime as the TABLE_SHARE itself. Neither tabledef_version.str and extra2 are good enough for that.
+ memcpy(buf, extra2, length); + tabledef_version.str= buf; + tabledef_version.length= length; + } + break; + default: + /* abort frm parsing if it's an unknown but important extra2 value */ + if (type >= 128)
128 should probably be a define.
Done.
+ goto err; + } + extra2+= length; + } + if (extra2 > e2end) + goto err;
If you have my earlier test, you don't need this one. Better to do the test early...
Of course. Added an early test.
- mysql_file_seek(file,pos,MY_SEEK_SET,MYF(0)); - if (mysql_file_read(file, forminfo,288,MYF(MY_NABP))) + if (!(pos= uint4korr(frm_image + 64 + len))) goto err; - share->frm_version= head[2]; + + forminfo= frm_image + pos;
Check that forminfo + FRM_FORM_LENGTH is inside of image
Done. Here and below too.
@@ -1420,7 +1360,6 @@ static int open_binary_frm(THD *thd, TAB geom_type= (Field::geometry_type) strpos[14]; charset= &my_charset_bin; #else - error= 4; // unsupported field type goto err; #endif
Shouldn't we get a better error for this case than just BAD_FRM ?
It was this way before, and I did not change it. See open_table_error() in 5.5: void open_table_error(TABLE_SHARE *share, int error, int db_errno, int errarg) { ... switch (error) { ... default: /* Better wrong error than none */ case 4: strxmov(buff, share->normalized_path.str, reg_ext, NullS); my_error(ER_NOT_FORM_FILE, errortype, buff); break; But see below about a special error message.
@@ -1578,10 +1523,8 @@ static int open_binary_frm(THD *thd, TAB (TYPELIB*) 0), share->fieldnames.type_names[i]); if (!reg_field) // Not supported field type - { - error= 4; - goto err; /* purecov: inspected */ - } + goto err; +
Shouldn't we get a better error for this case than just BAD_FRM ?
See above.
@@ -1607,20 +1550,8 @@ static int open_binary_frm(THD *thd, TAB if (reg_field->unireg_check == Field::NEXT_NUMBER) share->found_next_number_field= field_ptr;
- if (use_hash) - { - if (my_hash_insert(&share->name_hash, - (uchar*) field_ptr)) - { - /* - Set return code 8 here to indicate that an error has - occurred but that the error message already has been - sent (OOM). - */ - error= 8; - goto err; - } - } + if (use_hash && my_hash_insert(&share->name_hash, (uchar*) field_ptr)) + goto err;
Is this change because my_hash_insert() will give an error for OEM?
Yes.
@@ -1950,11 +1877,7 @@ static int open_binary_frm(THD *thd, TAB share->default_values, reg_field, &share->next_number_key_offset, &share->next_number_keypart)) < 0) - { - /* Wrong field definition */ - error= 4; - goto err; - } + goto err; // Wrong field definition else reg_field->flags |= AUTO_INCREMENT_FLAG;
As you are cleaning things up, remove else...
Done.
<cut>
+ case OPEN_FRM_OK: + DBUG_ASSERT(0); // open_table_error() is never called for this one
Add a break after the assert
Done.
<cut>
- case 6: - strxmov(buff, share->normalized_path.str, reg_ext, NullS); - my_printf_error(ER_NOT_FORM_FILE, - "Table '%-.64s' was created with a different version " - "of MySQL and cannot be read", - MYF(0), buff); - break;
It would be nice to still keep the above error message. We may need it sooner or later! For example, when we find a required table option type that we don't support! In this case we should also print out the MySQL version that was used to create the table!
This error was only used when FRM_VERSION was different, that is never. We can have a new error message for unknown field types (error=4 in 5.5) and unknown table options, etc. I'd agree it's a good idea. But it's independent from the error=6 above, which was pretty much a dead code.
- case 8: + case OPEN_FRM_NOT_A_TABLE: + my_error(ER_WRONG_OBJECT, MYF(0), share->db.str, + share->table_name.str, "TABLE"); break; - default: /* Better wrong error than none */ - case 4: + case OPEN_FRM_DISCOVER: + DBUG_ASSERT(0); // open_table_error() is never called for this one
Add a break! (Just in case)
Done.
+ case OPEN_FRM_CORRUPTED: strxmov(buff, share->normalized_path.str, reg_ext, NullS); my_error(ER_NOT_FORM_FILE, errortype, buff); break;
<cut>
+void prepare_fileinfo(THD *thd, uint reclength, uchar *fileinfo, + HA_CREATE_INFO *create_info, uint keys, KEY *key_info) +{
Change name to prepare_frm_header
Done.
- DBUG_ENTER("create_frm"); - - if (create_info->options & HA_LEX_CREATE_TMP_TABLE) - create_flags|= O_EXCL | O_NOFOLLOW;
I couldn't find where you ensured in writefrm that the above flags was added for temporary tables.
We really want to have these flags for temporary files to ensure that we fail if the file already exists! We also don't want anyone to create a symlink in /tmp and use that to overwrite another file!
Fixed.
@@ -3367,7 +3204,7 @@ rename_file_ext(const char * from,const char from_b[FN_REFLEN],to_b[FN_REFLEN]; (void) strxmov(from_b,from,ext,NullS); (void) strxmov(to_b,to,ext,NullS); - return (mysql_file_rename(key_file_frm, from_b, to_b, MYF(MY_WME))); + return (mysql_file_rename(key_file_frm, from_b, to_b, MYF(0)));
Why not give a warning if rename fales?
At least, use 'ME_JUST_WARNING'
Because it's neither a warning nor an error, it's a normal situation. For a discovering table some files with some extensions may not exist. If it's an error (like for a non-discovering engine) the upper layer will issue a proper error message.
=== modified file 'sql/table.h' --- sql/table.h 2013-01-29 14:10:47 +0000 +++ sql/table.h 2013-03-06 18:27:13 +0000
+/* Adjust number to next larger disk buffer */ +static inline ulong next_io_size(ulong pos) +{ + return MY_ALIGN(pos, IO_SIZE); +} +
Remove above function. Instead store true max length in frominfo+2
Done.
+++ sql/unireg.cc 2013-03-18 15:21:57 +0000
<cut>
+ frm.length= 64; // fileinfo;
Change 64 to FRM_HEADER_LENGTH (and use this in other part of the source)
Done.
+ frm.length+= extra2_size + 4; // mariadb extra2 frm segment + + int2store(fileinfo+4, extra2_size);
Add comment: // Size of table version and table options
It's the size of the extra2 segment. At the moment this segment only has the version and the table options, but I don't want a comment that needs to be updated every time we add something new to extra2. I can add a comment "Size of the extra2 segment", but I think the variable name and the comment on the previous line make it quite clear already, don't they?
Note that you can't use position 4 for this. Position 4 & 5 is reserved for the number of different forms/screens inside the frm. Using this would make the created table unusable for any old version of MySQL / MariaDB.
Already clarified on the phone.
+ int2store(fileinfo+6, frm.length);
Add comment: // Position to key information
Done.
+ frm.length+= key_buff_length; + frm.length+= reclength; // row with default values + frm.length+= create_info->extra_size; + + filepos= frm.length; + frm.length+= 288; // forminfo
Replace 288 in the code with FRM_FORM_LENGTH
Done.
+ frm.length+= packed_fields_length(create_fields); + + frm_ptr= (uchar*) my_malloc(frm.length, MYF(MY_WME | MY_ZEROFILL | + MY_THREAD_SPECIFIC)); + if (!frm_ptr) + DBUG_RETURN(frm); + + /* write the extra2 segment */ + pos = frm_ptr + 64;
64 -> FRM_HEADER_LENGTH
Done.
Please move the line: memcpy(frm_ptr, fileinfo, 64);
Before the above line. (Makes it easier to see that header is copied)
This memcpy line is put directly after the last update of the fileinfo buffer. I cannot move it up. Well, I can move it and modify frm_ptr[] directly instead of fileinfo[]. I'd rather not it, it's clearer, I think, when frm header is completely prepared in fileinfo[] and then copied, as compared to copying a partically prepared header and then finalizing it in-place.
+ *pos++ = EXTRA2_TABLEDEF_VERSION;
We probably want to add a comment that MySQL stores '/' here and that EXTRA2_TABLEDEF_VERSION can't never be '/'
Done. And a compile_time_assert(EXTRA2_TABLEDEF_VERSION != '/') too.
<cut>
+ DBUG_ASSERT(options_len <= 65535); // FIXME if necessary + int2store(pos + 1, options_len);
Why not just use (so that you can get rid of the comment) int3store(pos + 1, options_len);
It's not an issue. The length is written as follows: if it's < 255, then it's in the next byte. if it's >255 and <65535, then the next byte is 0 and the two following bytes have the length. The comment means that if we ever need to handle options_len > 64K, we should extend this length-encoding logic to support larger options_len. If I'd put int3store, as you suggest, the assert would've looked DBUG_ASSERT(options_len <= 16777216); // FIXME if necessary it wouldn't go away. But for now, I think, 64K is large enough. We can extend it when we need it.
+ pos+= 3;
+ } + pos= engine_table_options_frm_image(pos, create_info->option_list, + create_fields, keys, key_info); + } + + int4store(pos, filepos); // end of the extra2 segment + pos+= 4; + + DBUG_ASSERT(pos == frm_ptr + uint2korr(fileinfo+6)); + key_info_length= pack_keys(pos, keys, key_info, data_offset);
maxlength=(uint) next_io_size((ulong) (uint2korr(forminfo)+1000)); int2store(forminfo+2,maxlength);
You can skip the above 2 rows and mark the bytes as free to use.
I kept it, only removed next_io_size(). Even if unused - we don't need them now.
int4store(fileinfo+10,(ulong) (filepos+maxlength));
Same with the above 4 bytes
Nope, you wanted me to put the real frm length here to avoid fstat(). That's what I did. So these bytes are certainly not unused anymore.
<cut>
} } if (forminfo[46] == (uchar)255) {
Add comment /* New style MySQL 5.5 table comment */
Done.
By the way, this was a *stupid* solution for MySQL 5.5 from MySQL 6.0 What it means is that long comments from MySQL 5.5 are hidden in MySQL below 5.5
A better solution would be to always copy up to TABLE_COMMENT_INLINE_MAXLEN bytes into forminfo+47.
(Better to have a cut comment than no comment). Can you please do that change?
it's not doable in a compatible way. pre-5.5 servers expect to see frm length in forminfo[46], and it must be at most 60. 5.5 servers put 255 there to mark that the comment is in the extra segment. If I put a part of the comment in forminfo+47, what should I have in forminfo[46] ? If I don't put 255 - 5.5 servers won't see the long comment. If I *do* put 255 there, pre-5.5 servers will grab the whole 255 bytes starting from forminfo+47. Which will at best show garbage in the comment starting from 60th byte. At worst it'll crash, because forminfo is only 288 bytes long.
<cut>
+int rea_create_table(THD *thd, LEX_CUSTRING *frm, + const char *path, const char *db, const char *table_name, + HA_CREATE_INFO *create_info, handler *file) { DBUG_ENTER("rea_create_table");
- char frm_name[FN_REFLEN]; - strxmov(frm_name, path, reg_ext, NullS); - if (mysql_create_frm(thd, frm_name, db, table_name, create_info, - create_fields, keys, key_info, file)) + if (file) + { + // TODO don't write frm for temp tables + if (create_info->tmp_table() && + writefrm(path, db, table_name, 0, frm->str, frm->length)) + goto err_handler;
Can you add a test that if it's a memory table we don't write the frm? (Better to do it for other tmp tables, as we need to delete them from the engine during restart).
We do write frms for temporary tables now, as you see :) Fixing this is a separate task, I will do that after I push everything else that I have in my tree (and I hope to push today).
<cut>
@@ -718,10 +535,8 @@ static bool pack_header(uchar *forminfo, my_snprintf(warn_buff, sizeof(warn_buff), ER(ER_TOO_LONG_FIELD_COMMENT), field->field_name, static_cast<ulong>(COLUMN_COMMENT_MAXLEN)); - /* do not push duplicate warnings */ - if (!check_duplicate_warning(current_thd, warn_buff, strlen(warn_buff))) - push_warning(current_thd, MYSQL_ERROR::WARN_LEVEL_WARN, - ER_TOO_LONG_FIELD_COMMENT, warn_buff); + push_warning(current_thd, MYSQL_ERROR::WARN_LEVEL_WARN, + ER_TOO_LONG_FIELD_COMMENT, warn_buff);
You could clean up the above code at the same time. Now we duplicate a lot of code above.
->
bool strict= (current_thd->variables.sql_mode & (MODE_STRICT_TRANS_TABLES | MODE_STRICT_ALL_TABLES)));
my_error(ER_TOO_LONG_FIELD_COMMENT, MYF(strict ? 0 : ME_JUST_WARNING), field->field_name, static_cast<ulong>(COLUMN_COMMENT_MAXLEN)); if (strict) DBUG_RETURN(1); field->comment.length= tmp_len;
done.
@@ -833,7 +647,7 @@ static bool pack_header(uchar *forminfo, } /* Hack to avoid bugs with small static rows in MySQL */ reclength=max(file->min_record_length(table_options),reclength); - if (info_length+(ulong) create_fields.elements*FCOMP+288+ + if ((ulong) create_fields.elements*FCOMP+288+
288 -> FRM_FORM_LENGTH
done.
Should be:
length+= field->vcol_info->expr_str.length + FRM_VCOL_HEADER_SIZE(field->interval_id);
Why? This is the code where the byte is written in pack_fields(). (The code you copied was also wrong, see below)
if (field->interval_id) *buff++= (uchar) field->interval_id;
Instead, I've fixed this if() to say if (field->interval) because always when field->interval_id is not zero, field->interval is not zero too. But the opposite is not true, field->interval may be not zero, but field->interval_id may be zero (before get_interval_id() is called). Which means we cannot use if (field->interval_id) everywhere, and sometimes we have to use if (field->interval). But then, for consistency, it's better to use if (field->interval) everywhere.
@@ -738,56 +748,40 @@ int ha_archive::create(const char *name, <cut> + /* + Here is where we open up the frm and pass it to archive to store + */ + if (!table_arg->s->read_frm_image(&frm_ptr, &frm_len)) + { + azwrite_frm(&create_stream, frm_ptr, frm_len); + my_free(const_cast<uchar*>(frm_ptr)); + }
Generally it would be better to have a function:
table_arg->s->free_frm_image(&frm_ptr)
Because the server could be compiled with another memory allocator than the storage engine (DBUG / NO_DBUG).
Right. Fixed.
=== modified file 'storage/federatedx/ha_federatedx.cc' --- storage/federatedx/ha_federatedx.cc 2013-01-23 15:24:04 +0000 +++ storage/federatedx/ha_federatedx.cc 2013-03-10 10:49:49 +0000 @@ -3588,6 +3574,71 @@ int ha_federatedx::rollback(handlerton * +int ha_federatedx::discover_assisted(handlerton *hton, THD* thd, + TABLE_SHARE *table_s, HA_CREATE_INFO *info)
<cut>
+ query.copy(rdata[1], rlen[1], cs); + query.append(STRING_WITH_LEN(" CONNECTION=\""), cs); + query.append(table_s->connect_string.str, table_s->connect_string.length, cs); + query.append('"');
Shouldn't we need to quote the string properly? (It may have " inside)
Fixed.
Other things:
- Please run sql_bench/test-create on your tree to verify that things are now faster (should be as we don't have to open a frm when a table is created). - Test it also for temporary memory tables (assuming you can get rid of the .frm for them)
After the push. Regards, Sergei
Hi!
"Sergei" == Sergei Golubchik <serg@askmonty.org> writes:
<cut>
+ if (my_fstat(file, &stats, MYF(0))) + goto err;
The above I don't like.
We already store the max possible length of the frm in frm_header+10. Why not store the exact length there and use this instead of doing fstat()? Would save us a file operation (at least for MariaDB files). It would also ensure that we don't easily break if we find a partial .frm file on disk.
Sergei> Done. Also added a limit, as we discussed: Perfect!
+ switch (type) { + case EXTRA2_TABLEDEF_VERSION: + if (tabledef_version.str) // see init_from_sql_statement_string() + { + if (length != tabledef_version.length || + memcmp(extra2, tabledef_version.str, length)) + goto err; + } + else + { + uchar *buf= (uchar*) alloc_root(&mem_root, length); + if (!buf) + goto err;
You could have set tabledef_version.str directly and not use buf.
Sergei> How comes? We need to copy the version into TABLE_SHARE's memroot, to Sergei> make sure it has the same lifetime as the TABLE_SHARE itself. Sergei> Neither tabledef_version.str and extra2 are good enough for that.
+ memcpy(buf, extra2, length); + tabledef_version.str= buf;
I meant that you could have used: if (!tabledef_version.str= (uchar*) memdup_root(&mem_root, extra2, length)) goto err; Instead of introducing a new variable that one has to keep track of when one reads the code.
<cut>
- case 6: - strxmov(buff, share->normalized_path.str, reg_ext, NullS); - my_printf_error(ER_NOT_FORM_FILE, - "Table '%-.64s' was created with a different version " - "of MySQL and cannot be read", - MYF(0), buff); - break;
It would be nice to still keep the above error message. We may need it sooner or later! For example, when we find a required table option type that we don't support! In this case we should also print out the MySQL version that was used to create the table!
Sergei> This error was only used when FRM_VERSION was different, that is never. Yes, before this was not the case, but it will be in the future when we add new things that old MariaDB versions can't read. Sergei> We can have a new error message for unknown field types (error=4 in 5.5) Sergei> and unknown table options, etc. I'd agree it's a good idea. Sergei> But it's independent from the error=6 above, which was pretty much a Sergei> dead code. Yes, error 6 was bad.
@@ -3367,7 +3204,7 @@ rename_file_ext(const char * from,const char from_b[FN_REFLEN],to_b[FN_REFLEN]; (void) strxmov(from_b,from,ext,NullS); (void) strxmov(to_b,to,ext,NullS); - return (mysql_file_rename(key_file_frm, from_b, to_b, MYF(MY_WME))); + return (mysql_file_rename(key_file_frm, from_b, to_b, MYF(0)));
Why not give a warning if rename fales?
At least, use 'ME_JUST_WARNING'
Sergei> Because it's neither a warning nor an error, it's a normal situation. Sergei> For a discovering table some files with some extensions may not exist. At least, add it the flag to rename_file_ext() so that the caller can decide if there should be a warning or not. (We don't want any silent failures, do we) The caller can't know if the above succeeded or not. (For example because of a permission error). Sergei> If it's an error (like for a non-discovering engine) the upper layer Sergei> will issue a proper error message. <cut>
Please move the line: memcpy(frm_ptr, fileinfo, 64);
Before the above line. (Makes it easier to see that header is copied)
Sergei> This memcpy line is put directly after the last update of the fileinfo Sergei> buffer. I cannot move it up. Sergei> Well, I can move it and modify frm_ptr[] directly instead of fileinfo[]. Sergei> I'd rather not it, it's clearer, I think, when frm header is completely Sergei> prepared in fileinfo[] and then copied, as compared to copying a Sergei> partically prepared header and then finalizing it in-place. You can always do: frm_ptr= my_malloc(...) mecmpy(frm_ptr, file_info, 64); file_info= frm_ptr; (basicly)
<cut>
+ DBUG_ASSERT(options_len <= 65535); // FIXME if necessary + int2store(pos + 1, options_len);
Why not just use (so that you can get rid of the comment) int3store(pos + 1, options_len);
Sergei> It's not an issue. The length is written as follows: Sergei> if it's < 255, then it's in the next byte. Sergei> if it's >255 and <65535, then the next byte is 0 and the Sergei> two following bytes have the length. Sergei> The comment means that if we ever need to handle options_len > 64K, we Sergei> should extend this length-encoding logic to support larger options_len. Sergei> If I'd put int3store, as you suggest, the assert would've looked Sergei> DBUG_ASSERT(options_len <= 16777216); // FIXME if necessary Sergei> it wouldn't go away. Sergei> But for now, I think, 64K is large enough. We can extend it when we need Sergei> it. Then please add a better comment. I thought that FIXME meant that someone should fix the code ASAP. <cut>
You can skip the above 2 rows and mark the bytes as free to use.
Sergei> I kept it, only removed next_io_size(). Sergei> Even if unused - we don't need them now.
int4store(fileinfo+10,(ulong) (filepos+maxlength));
Same with the above 4 bytes
Sergei> Nope, you wanted me to put the real frm length here to avoid fstat(). Sergei> That's what I did. So these bytes are certainly not unused anymore. Good!
<cut>
} } if (forminfo[46] == (uchar)255) {
Add comment /* New style MySQL 5.5 table comment */
Sergei> Done.
By the way, this was a *stupid* solution for MySQL 5.5 from MySQL 6.0 What it means is that long comments from MySQL 5.5 are hidden in MySQL below 5.5
A better solution would be to always copy up to TABLE_COMMENT_INLINE_MAXLEN bytes into forminfo+47.
(Better to have a cut comment than no comment). Can you please do that change?
Sergei> it's not doable in a compatible way. Sergei> pre-5.5 servers expect to see frm length in forminfo[46], and it must be at Sergei> most 60. Sergei> 5.5 servers put 255 there to mark that the comment is in the extra Sergei> segment. Sergei> If I put a part of the comment in forminfo+47, what should I have in Sergei> forminfo[46] ? If I don't put 255 - 5.5 servers won't see the long Sergei> comment. If I *do* put 255 there, pre-5.5 servers will grab the whole Sergei> 255 bytes starting from forminfo+47. Which will at best show garbage in Sergei> the comment starting from 60th byte. At worst it'll crash, because Sergei> forminfo is only 288 bytes long. Agree that we can't store the length there. Sorry for a not working idea. But I still think they could have done it better in MySQL, like storing the first 60 characters in frominfo+47 and the rest in the extra segment... Regards, Monty
Hi, Michael! On Apr 08, Michael Widenius wrote:
"Sergei" == Sergei Golubchik <serg@askmonty.org> writes:
I meant that you could have used:
if (!tabledef_version.str= (uchar*) memdup_root(&mem_root, extra2, length)) goto err;
Done.
@@ -3367,7 +3204,7 @@ rename_file_ext(const char * from,const char from_b[FN_REFLEN],to_b[FN_REFLEN]; (void) strxmov(from_b,from,ext,NullS); (void) strxmov(to_b,to,ext,NullS); - return (mysql_file_rename(key_file_frm, from_b, to_b, MYF(MY_WME))); + return (mysql_file_rename(key_file_frm, from_b, to_b, MYF(0)));
Why not give a warning if rename fales?
At least, use 'ME_JUST_WARNING'
Because it's neither a warning nor an error, it's a normal situation. For a discovering table some files with some extensions may not exist.
At least, add it the flag to rename_file_ext() so that the caller can decide if there should be a warning or not. (We don't want any silent failures, do we)
The caller can't know if the above succeeded or not. (For example because of a permission error).
The caller does: if (rename_file_ext(from, to, *ext)) { if ((error=my_errno) != ENOENT) break; error= 0; } A permission error will be noticed and reported, as far as I can see.
The comment means that if we ever need to handle options_len > 64K, we should extend this length-encoding logic to support larger options_len. If I'd put int3store, as you suggest, the assert would've looked DBUG_ASSERT(options_len <= 16777216); // FIXME if necessary it wouldn't go away.
But for now, I think, 64K is large enough. We can extend it when we need it.
Then please add a better comment. I thought that FIXME meant that someone should fix the code ASAP.
Done. Regards, Sergei
participants (2)
-
Michael Widenius -
Sergei Golubchik