April 2013 - developers - lists.mariadb.org

[Maria-developers] Review of last part of discover code
by Michael Widenius 08 Apr '13

08 Apr '13

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

2 3

[Maria-developers] KB Question: Storage engine plugin that "proxies" another engine?
by Daniel Bartholomew 08 Apr '13

08 Apr '13

The following question has been posted to the AskMonty Knowledgebase: https://kb.askmonty.org/en/storage-engine-plugin-that-proxies-another-engin… - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - Storage engine plugin that "proxies" another engine? I would like to write a plugin that adds transparent versioning support for table rows. The idea is one would create a table and specify something like "Engine=Versioned", which would create two regular tables in InnoDB/MyISAM. One table to hold the current data, and one to hold a couple of previous versions. Then, when running UPDATE/DELETE statements, the "versioned" engine would first copy the current data to the "history" table and then forward the statement to the underlying storage engine. Is this possible to do in a nice way using the plugin architecture? Thanks Thomas - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - Thanks. -- Daniel Bartholomew Google+ - http://gplus.to/dbart Twitter - http://twitter.com/daniel_bart MariaDB: An Enhanced Drop-in Replacement for MySQL Website - http://mariadb.org Twitter - http://twitter.com/mariadb Google+ - http://google.com/+mariadb Facebook - http://fb.com/MariaDB.dbms Knowledgebase - http://kb.askmonty.org Monty Program - http://montyprogram.com

1 0

[Maria-developers] mysql test question
by Rich Prohaska 05 Apr '13

05 Apr '13

Hello, I was wondering if there was a way to run a mysql-test on with a prebuilt data directory. If so, can you point me to some example? Thanks Rich Prohaska

3 4

[Maria-developers] Rev 3712: MDEV-4338 : Support atomic option on directFS/FusionIO
by Sergei Golubchik 05 Apr '13

05 Apr '13

Hi, Vladislav! On Mar 29, Vladislav Vaintroub wrote: > ------------------------------------------------------------ > revno: 3712 > revision-id: wlad(a)montyprogram.com-20130328203348-1qe9u9nlq6ol9b8l > parent: igor(a)askmonty.org-20130329021836-x6swah73a59im9tn > fixes bug: https://mariadb.atlassian.net/browse/MDEV-4338 > committer: Vladislav Vaintroub <wlad(a)montyprogram.com> > branch nick: 5.5 > timestamp: Thu 2013-03-28 21:33:48 +0100 > message: > MDEV-4338 : Support atomic option on directFS/FusionIO. > === modified file 'storage/xtradb/handler/ha_innodb.cc' > --- a/storage/xtradb/handler/ha_innodb.cc 2013-03-08 18:08:45 +0000 > +++ b/storage/xtradb/handler/ha_innodb.cc 2013-03-28 20:33:48 +0000 > @@ -185,6 +185,7 @@ static my_bool innobase_file_format_chec > static my_bool innobase_log_archive = FALSE; > static char* innobase_log_arch_dir = NULL; > #endif /* UNIV_LOG_ARCHIVE */ > +static my_bool innobase_use_atomic_writes = TRUE; > static my_bool innobase_use_doublewrite = TRUE; > static my_bool innobase_use_checksums = TRUE; > static my_bool innobase_fast_checksum = FALSE; > @@ -3057,6 +3058,35 @@ innobase_init( > srv_kill_idle_transaction = 0; > #endif > > + srv_use_atomic_writes = (ibool) innobase_use_atomic_writes; Is that the typical usage pattern in the innodb code? Because it looks a bit silly to me, why couldn't you create a sysvar directly on srv_use_atomic_writes, why an intermediate innobase_use_atomic_writes variable? > + if (innobase_use_atomic_writes) { > + fprintf(stderr, "InnoDB: using atomic writes.\n"); > + > + /* Force doublewrite buffer off, atomic writes replace it. */ > + if (srv_use_doublewrite_buf) { > + fprintf(stderr, "InnoDB: Switching off doublewrite buffer " > + "because of atomic writes.\n"); > + innobase_use_doublewrite = srv_use_doublewrite_buf = FALSE; > + } > + > + /* Force O_DIRECT on Unixes (on Windows writes are always unbuffered)*/ > +#ifndef _WIN32 > + if(!innobase_file_flush_method || > + !strstr(innobase_file_flush_method, "O_DIRECT")) { > + innobase_file_flush_method = > + srv_file_flush_method_str = (char*)"O_DIRECT"; > + fprintf(stderr, "InnoDB: using O_DIRECT due to atomic writes.\n"); > + } > +#endif > +#ifdef HAVE_POSIX_FALLOCATE > + /* Due to a bug in directFS, using atomics needs > + * posix_fallocate to extend the file > + * pwrite() past end of the file won't work > + */ > + srv_use_posix_fallocate = TRUE; > +#endif > + } it'd be nice if you could try os_file_set_atomic_writes() here to see if that works. This function creates and opens quite a few files, you could use one of them to mark it for atomic writes, and if that would fail, you'd disable atomic writes, and wouldn't change innobase_file_flush_method and innobase_use_doublewrite. Btw, why not to use posix_fallocate whenever it's available? Or, at least, with its own --innodb-use-fallocate option? Regards, Sergei

3 6

Re: [Maria-developers] MDEV-3917 multiple use locks (GET_LOCK) in one connection
by Sergei Golubchik 04 Apr '13

04 Apr '13

Hi, Holyfoot! (Kostja - there're a couple of questions at the end that you might want to reply to) On Apr 02, holyfoot(a)askmonty.org wrote: > At file:///home/hf/wmar/mdev-3917/ > > ------------------------------------------------------------ > revno: 3510 > revision-id: holyfoot(a)askmonty.org-20130402085219-1sxuzssq7segeig4 > parent: igor(a)askmonty.org-20130331221855-1n7hagklb3soplxq > committer: Alexey Botchkov <holyfoot(a)askmonty.org> > branch nick: mdev-3917 > timestamp: Tue 2013-04-02 13:52:19 +0500 > message: > MDEV-3917 multiple use locks (GET_LOCK) in one connection. > The patch contributed by Konstantin Osipov applied. > Native comments: > Implement multiple user-level locks per connection. > > GET_LOCK() function in MySQL allows a connection to hold at most > one user level lock. Taking a new lock automatically releases the > old lock, if any. > > The limit of one lock per session existed since early versions > of MySQL didn't have a deadlock detector for SQL locks. > MDL patches in MySQL 5.5 added a deadlock detector, > so starting from 5.5 it became possible to take multiple locks > in any order -- a deadlock, should it occur, would be detected > and an error returned to the client which closed the wait chain. > > This is exactly what is done in this patch: ULLs are moved > to use MDL subsystem. > > also: > MDL_key::USER renamed to MDL::USER_LOCK What does that mean? > boundary checks for user lock name added. > === modified file 'sql/item_func.cc' > --- a/sql/item_func.cc 2013-03-27 22:41:02 +0000 > +++ b/sql/item_func.cc 2013-04-02 08:52:19 +0000 > @@ -4010,7 +3896,121 @@ int Interruptible_wait::wait(mysql_cond_ > > > /** > - Get a user level lock. If the thread has an old lock this is first released. > + For locks with EXPLICIT duration, MDL returns a new ticket > + every time a lock is granted. This allows to implement recursive > + locks without extra allocation or additional data structures, such > + as below. However, if there are too many tickets in the same > + MDL_context, MDL_context::find_ticket() is getting too slow, > + since it's using a linear search. > + This is why a separate structure is allocated for a user > + level lock, and before requesting a new lock from MDL, > + GET_LOCK() checks thd->ull_hash if such lock is already granted, > + and if so, simply increments a reference counter. > +*/ > + > +class User_level_lock > +{ > +public: > + MDL_ticket *lock; > + int refs; > +}; > + > + > +/** Extract a hash key from User_level_lock. */ > + > +uchar *ull_get_key(const uchar *ptr, size_t *length, > + my_bool not_used __attribute__((unused))) > +{ > + User_level_lock *ull = (User_level_lock*) ptr; > + MDL_key *key = ull->lock->get_key(); > + *length= key->length(); > + return (uchar*) key->ptr(); > +} > + > + > +/** > + Release all user level locks for this THD. > +*/ > + > +void mysql_ull_cleanup(THD *thd) > +{ > + User_level_lock *ull; > + DBUG_ENTER("mysql_ull_cleanup"); > + > + for (uint i= 0; i < thd->ull_hash.records; i++) > + { > + ull = (User_level_lock*) my_hash_element(&thd->ull_hash, i); > + thd->mdl_context.release_lock(ull->lock); > + free(ull); my_free > + } > + > + my_hash_free(&thd->ull_hash); > + > + DBUG_VOID_RETURN; > +} > + > + > +/** > + Set explicit duration for metadata locks corresponding to > + user level locks to protect them from being released at the end > + of transaction. > +*/ > + > +void mysql_ull_set_explicit_lock_duration(THD *thd) > +{ > + User_level_lock *ull; > + DBUG_ENTER("mysql_ull_set_explicit_lock_duration"); > + > + for (uint i= 0; i < thd->ull_hash.records; i++) > + { > + ull= (User_level_lock*) my_hash_element(&thd->ull_hash, i); > + thd->mdl_context.set_lock_duration(ull->lock, MDL_EXPLICIT); > + } > + DBUG_VOID_RETURN; > +} > + > + > +/** > + When MDL detects a lock wait timeout, it pushes > + an error into the statement diagnostics area. > + For GET_LOCK(), lock wait timeout is not an error, > + but a special return value (0). NULL is returned in > + case of error. > + Capture and suppress lock wait timeout. > +*/ > + > +class Lock_wait_timeout_handler: public Internal_error_handler > +{ > +public: > + Lock_wait_timeout_handler() :m_lock_wait_timeout(false) {} > + > + bool m_lock_wait_timeout; > + > + bool handle_condition(THD * /* thd */, uint sql_errno, > + const char * /* sqlstate */, > + MYSQL_ERROR::enum_warning_level /* level */, > + const char *message, > + MYSQL_ERROR ** /* cond_hdl */); > +}; > + > +bool > +Lock_wait_timeout_handler:: > +handle_condition(THD * /* thd */, uint sql_errno, > + const char * /* sqlstate */, > + MYSQL_ERROR::enum_warning_level /* level */, > + const char *message, > + MYSQL_ERROR ** /* cond_hdl */) > +{ > + if (sql_errno == ER_LOCK_WAIT_TIMEOUT) > + { > + m_lock_wait_timeout= true; > + return true; /* condition handled */ > + } > + return false; > +} > + > +/** > + Get a user level lock. > > @retval > 1 : Got lock > @@ -4039,104 +4038,75 @@ longlong Item_func_get_lock::val_int() > it's not guaranteed to be same as on master. > */ > if (thd->slave_thread) > + { > + null_value= 0; > DBUG_RETURN(1); > + } > > if (!res || !res->length()) > + DBUG_RETURN(0); > + > + if (res->length() > NAME_LEN) > { > + my_error(ER_TOO_LONG_IDENT, MYF(0), res->c_ptr_safe()); > DBUG_RETURN(0); > } > + > DBUG_PRINT("info", ("lock %.*s, thd=%ld", res->length(), res->ptr(), > (long) thd->real_id)); > + /* HASH entries are of type User_level_lock. */ > + if (! my_hash_inited(&thd->ull_hash) && > + my_hash_init(&thd->ull_hash, &my_charset_bin, > + 16 /* small hash */, 0, 0, ull_get_key, NULL, 0)) > { > + DBUG_RETURN(0); > } > > + MDL_request ull_request; > + ull_request.init(MDL_key::USER_LOCK, res->c_ptr_safe(), "", > + MDL_SHARED_NO_WRITE, MDL_EXPLICIT); > + MDL_key *ull_key = &ull_request.key; > > > + if ((ull= (User_level_lock*) > + my_hash_search(&thd->ull_hash, ull_key->ptr(), ull_key->length()))) > + { > + /* Recursive lock */ > + ull->refs++; > + null_value = 0; > + DBUG_RETURN(1); > + } > > + Lock_wait_timeout_handler lock_wait_timeout_handler; > + thd->push_internal_handler(&lock_wait_timeout_handler); > + bool error= thd->mdl_context.acquire_lock(&ull_request, timeout); > + (void) thd->pop_internal_handler(); > + if (error) > { > + if (lock_wait_timeout_handler.m_lock_wait_timeout) > + null_value= 0; > + DBUG_RETURN(0); > } > > + ull= (User_level_lock*) malloc(sizeof(User_level_lock)); my_malloc() with MYF(MY_WME|MY_THREAD_SPECIFIC) > + if (ull == NULL) > { > + thd->mdl_context.release_lock(ull_request.ticket); > + DBUG_RETURN(0); > } > + > + ull->lock= ull_request.ticket; > + ull->refs= 1; > + > + if (my_hash_insert(&thd->ull_hash, (uchar*) ull)) > { > + thd->mdl_context.release_lock(ull->lock); > + free(ull); my_free > + DBUG_RETURN(0); > } > + null_value= 0; > > + DBUG_RETURN(1); > } > > > @@ -4151,43 +4121,104 @@ longlong Item_func_get_lock::val_int() > longlong Item_func_release_lock::val_int() > { > DBUG_ASSERT(fixed == 1); > + String *res= args[0]->val_str(&value); > + THD *thd= current_thd; > DBUG_ENTER("Item_func_release_lock::val_int"); > + null_value= 1; > + > if (!res || !res->length()) > + DBUG_RETURN(0); > + > + if (res->length() > NAME_LEN) > { > + my_error(ER_TOO_LONG_IDENT, MYF(0), res->c_ptr_safe()); > DBUG_RETURN(0); > } > + > DBUG_PRINT("info", ("lock %.*s", res->length(), res->ptr())); > > + MDL_key ull_key; > + ull_key.mdl_key_init(MDL_key::USER_LOCK, res->c_ptr_safe(), ""); > + > + User_level_lock *ull; > + > + if (!(ull= > + (User_level_lock*) my_hash_search(&thd->ull_hash, > + ull_key.ptr(), ull_key.length()))) > { > + null_value= thd->mdl_context.get_lock_owner(&ull_key) == 0; > + DBUG_RETURN(0); > } > + null_value= 0; > + if (--ull->refs == 0) > { > + my_hash_delete(&thd->ull_hash, (uchar*) ull); > + thd->mdl_context.release_lock(ull->lock); > + free(ull); my_free > } > + DBUG_RETURN(1); > +} > + > + > +/** > + Check a user level lock. > + > + Sets null_value=TRUE on error. > + > + @retval > + 1 Available > + @retval > + 0 Already taken, or error > +*/ > + > +longlong Item_func_is_free_lock::val_int() > +{ > + DBUG_ASSERT(fixed == 1); > + String *res= args[0]->val_str(&value); > + THD *thd= current_thd; > + null_value= 1; > + this block ----- from here > + if (!res || !res->length()) > + return 0; > + > + if (res->length() > NAME_LEN) > + { > + my_error(ER_TOO_LONG_IDENT, MYF(0), res->c_ptr_safe()); > + return 0; > + } and up ---- to here is repeated four times. please move it to a separate function. Like if (!ull_name_ok(res)) return 0; > + > + MDL_key ull_key; > + ull_key.mdl_key_init(MDL_key::USER_LOCK, res->c_ptr_safe(), ""); > + > + null_value= 0; > + return thd->mdl_context.get_lock_owner(&ull_key) == 0; > +} > + > + > +longlong Item_func_is_used_lock::val_int() > +{ > + DBUG_ASSERT(fixed == 1); > + String *res= args[0]->val_str(&value); > + THD *thd= current_thd; > + null_value= 1; > + > + if (!res || !res->length()) > + return 0; > + > + if (res->length() > NAME_LEN) > + { > + my_error(ER_TOO_LONG_IDENT, MYF(0), res->c_ptr_safe()); > + return 0; > + } > + > + MDL_key ull_key; > + ull_key.mdl_key_init(MDL_key::USER_LOCK, res->c_ptr_safe(), ""); > + ulong thread_id = thd->mdl_context.get_lock_owner(&ull_key); > + if (thread_id == 0) > + return 0; > + > + null_value= 0; > + return thread_id; > } > > === modified file 'sql/mdl.cc' > --- a/sql/mdl.cc 2013-01-29 14:10:47 +0000 > +++ b/sql/mdl.cc 2013-04-02 08:52:19 +0000 > @@ -2094,8 +2151,6 @@ MDL_context::acquire_lock(MDL_request *m > > find_deadlock(); > > - if (lock->needs_notification(ticket)) > - { okay, so before the patch only locks that needs_notification were using the loop with short waits. Now all locks do. Presumably to check whether thd_is_connected still. Before the patch, how were client disconnects handled? Like, one could have started to wait on the mdl lock, and then disconnect. Was the wait aborted? > struct timespec abs_shortwait; > set_timespec(abs_shortwait, 1); > wait_status= MDL_wait::EMPTY; > @@ -2106,17 +2163,25 @@ MDL_context::acquire_lock(MDL_request *m > > if (wait_status != MDL_wait::EMPTY) > break; > + /* Check if the client is gone while we were waiting. */ > + if (! thd_is_connected(m_thd)) > + { > + /* > + * The client is disconnected. Don't wait forever: > + * assume it's the same as a wait timeout, this > + * ensures all error handling is correct. > + */ > + wait_status= MDL_wait::TIMEOUT; > + break; > + } > > mysql_prlock_wrlock(&lock->m_rwlock); > + if (lock->needs_notification(ticket)) why this if() is under the prlock, and not the other way around? > lock->notify_conflicting_locks(this); > mysql_prlock_unlock(&lock->m_rwlock); > set_timespec(abs_shortwait, 1); > } > if (wait_status == MDL_wait::EMPTY) > - wait_status= m_wait.timed_wait(m_thd, &abs_timeout, TRUE, > - mdl_request->key.get_wait_state_name()); > - } > - else > wait_status= m_wait.timed_wait(m_thd, &abs_timeout, TRUE, > mdl_request->key.get_wait_state_name()); Regards, Sergei

1 0

[Maria-developers] MDEV-4145/MWL#253 review feedback (unfinished)
by Sergei Petrunia 03 Apr '13

03 Apr '13

Hello Igor, Please find some review feedback below. ===== sql_statistics.cc has this line with comment: > If the value the variable 'use_stat_tables' is set to > "preferably" the optimizer uses a particular statistical data only if > it can't be found in the statistical data. the part > uses a particular statistical data > only if it can't be found > in the statistical data. doesn't seem to make any sense. Is this a typo? ===== sql_statistics.cc: collect_statistics_for_table() has this code: > table_field->collected_stats->cleanup(); > } > bitmap_clear_all(table->write_set); Why suddenly no indentation on the line with the bitmap_clear_all() call? This looks as if a merge has gone wrong, please fix. ===== sql_statistics.cc has this : > class Table_stat: public Stat_table > { > > private: > > Field *db_name_field; /* Field for the column table_stats.db_name */ Why do all class definitions start with 'private:'? It is active by default in C++. The code looks as if it was auto-translated from some other language. ===== I see that my progress reporting patch from November is not part of the tree yet. ===== I don't any code that would check if table DDLs of statistical tables are what the server expects them to be. I think the check should be made, just like it is made for mysql.proc and other tables. ==== Then I started to play with the code and have filed MDEV-4350, MDEV-4357, MDEV-4359, MDEV-4360, MDEV-4362, MDEV-4363, MDEV-4364. I find it difficult to proceed when there are so many simple counterexamples. BR Sergei -- Sergei Petrunia, Software Developer Monty Program AB, http://askmonty.org Blog: http://s.petrunia.net/blog

1 0

[Maria-developers] read_buffer_size query
by Ian Gilfillan 02 Apr '13

02 Apr '13

Hi I'm working on adding complete descriptions to the knowledgebase for all the server variables right now, and have a query on the read_buffer_size variable. The default in my MariaDB 10.0.1 seems to be 2097152, but it's listed on the MySQL docs with a default of 131072. I can't find a mention in the changelogs of this having changed - can anyone tell me in which version the change in default happened? Also, in the MySQL documentation, this variable is described as applying to MyISAM tables, while in the only short description on the Knowledgebase I can find, at https://kb.askmonty.org/en/mysqld-options-full-list/, it's described more generally, not specifically mentioning MyISAM. Does this variable apply to other storage engines as well now (Aria?), or still only MyISAM? thanks, ian gilfillan

3 4

[Maria-developers] Cassandra disabled due to excessive memory usage causing test failures
by Kristian Nielsen 02 Apr '13

02 Apr '13

Hi, When I added my 10.0-mdev26 tree to buildbot, I got some strange errors on the kvm-fulltest2 builder. I finally tracked it down to be caused by the OOM killer killing random processes. The root issue is Cassandra. At some point it seems cassandra was enabled in the VM used by the fulltest2 builder. However, just starting up Cassandra creates a jsvc process that uses 3.4 _gigabyte_ of memory, 1.3 GB of which is resident. That is too much... I also noticed that cassandra tests are not even run, plugins.cassandra is skipped due to cassandra not running. Possibly this is because the OOM killer has killed the process ;-). In any case, for now I have disabled cassandra on the -fulltest VMs: sudo update-rc.d cassandra disable If someone wants to re-enable it, feel free, but please first make sure that it does not cause unrelated test failures due to excessive memory usage! I would suggest not to run cassandra by default, instead add a separate buildbot step that explicitly starts up cassandra when needed. ---- In general, I am disappointed about the state of our trees. This problem seems to have been there for more than a month without anyone doing anything, and there are tons of other test failures in 10.0 and 10.0-base. We need to tighten up our processes here. Remember, the rules are that anything major should be green in a feature tree before pushing to a main tree. Clearly this is not what is happening at the moment. It's ridiculous to constantly hear people pressing for release of 10.0.2-beta when our trees are not even alpha state ... - Kristian.

1 0

[Maria-developers] I have a friend. Her name is Maria
by Кукушкин Александр 01 Apr '13

01 Apr '13

Hello! excuse me for off topic. I have a friend, her name is Maria and she has a birthday today. I thought maybe someone of you could write a commit message mentioning her. Wish her good performance and persistence or something like that. Then I could just show her a page like that one http://bazaar.launchpad.net/~maria-captains/maria/10.0/revision/3505 What do you think about that? is it possible? (sorry if this message is buplicate, I tried to post it yesterday to your mailing list but it seems like I failed)

1 1