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