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