maria-developers team mailing list archive

[Michael Widenius <monty@xxxxxxxxxxxx>]

Hi!

>>>>> "Sergei" == Sergei Golubchik <serg@xxxxxxxxxxxx> 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