maria-developers team mailing list archive
-
maria-developers team
-
Mailing list archive
-
Message #05331
Re: Review of last part of discover code
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
Follow ups
References
