From: Sergey Vojtovich
Date: July 27 2010 8:25am
Subject: Re: bzr commit into mysql-trunk-bugfixing branch (davi:3133) Bug#45377
List-Archive: http://lists.mysql.com/commits/114404
Message-Id: <20100727082547.GA13149@june>
MIME-Version: 1.0
Content-Type: text/plain; CHARSET=US-ASCII
Content-Transfer-Encoding: 7BIT

Hi Davi,

On Mon, Jul 26, 2010 at 12:17:29PM -0300, Davi Arnaut wrote:
> Hi Sergey,
> 
> On 7/26/10 9:12 AM, Sergey Vojtovich wrote:
> >Hi Davi,
> >
> >see comments inline.
> >
> >On Fri, Jul 23, 2010 at 12:04:25PM +0000, Davi Arnaut wrote:
> >># At a local mysql-trunk-bugfixing repository of davi
> >>
> >>  3133 Davi Arnaut	2010-07-23
> >>       Bug#45377: ARCHIVE tables aren't discoverable after OPTIMIZE
> >>
> >>       The problem was that the optimize method of the ARCHIVE storage
> >>       engine was not preserving the FRM embedded in the ARZ file when
> >>       rewriting the ARZ file for optimization. The ARCHIVE engine stores
> >>       the FRM in the ARZ file so it can be transferred from machine to
> >>       machine without also copying the FRM -- the engine restores the
> >>       embedded FRM during discovery.
> >>
> >>       The solution is to copy over the FRM when rewriting the ARZ file.
> >>       In addition, some initial error checking is performed to ensure
> >>       garbage is not copied over.
...skip...

> >>+  my_errno= 0;
> >I'm against this way of use of my_errno - it hides mistakes. Either
> >ensure that azread_frm() and azwrite_frm() set my_errno properly or
> >make them return error code.
> 
> Since my_errno is just a wrapper around errno and that errno is
> reserved for for system calls and library functions, this kind of
> construct is quite common, sometimes even required, to detect
> failures.
> 
> This is actually a recommend practice, see:
> 
> http://tinyurl.com/2d49xca
Ok, so I assume we have the following case here:
<quot>
Library Functions that don't promise to set errno

Some functions lack documentation regarding errno in the C99 standard.

For instance, the setlocale() function normally returns NULL in the
event of an error but does not guarantee setting errno.

After calling one of these functions, a program should not solely rely
on the value of errno to determine if an error occurred. The function
might have altered errno, but this does not promise that errno will
properly indicate an error condition.
</quot>

I'd say it is corrupt logic and we should avoid using it with code
that is under our control. setlocale(), as mentioned in the example
above, is different case: it is corrupt and we cannot do much here
but play according to it's rules.

> >>+
> >>+  /*
> >>+    Read file offset is repositioned when writing a new header.
> >>+    Write file offset is set to the end of the file.
> >>+  */
> >>+  if (azread_frm(&archive, frm_ptr) ||
> >>+      azwrite_frm(&writer, frm_ptr, archive.frm_length))
> >>+  {
> >>+    rc= my_errno ? my_errno : HA_ERR_INTERNAL_ERROR;
> >>+    goto error;
> >>+  }
> >>+
> >Ehm. I wouldn't insist, but... I doubt it is a good idea to copy
> >frm blob from old archive data file. There should be a couple of
> 
> No disagreement that it is a bad idea. But we either make it work or
> we remove this broken feature. What's it gonna be?
Ok, that's why I wouldn't insist. We'd better keep them in sync
instead of synchronizing on ::optimize(). But taking into account
nature of ARCHIVE engine, that means that every meta-data change
against ARCHIVE table would cause table copy.

Back in 5.1 I can remember a couple of cases when meta-data change
was not communicated to storage engine like column renames, default
value change, comment change, some upgrades, etc...

Things have been greatly improved with 5.5, but I don't see that
ARCHIVE utilze this new functionality.

Anyways, that's something not to be fixed within the scope of this
bug. Let's stick to the more convenient fix for now.

Regards,
Sergey
-- 
Sergey Vojtovich <svoj@stripped>
MySQL AB, Software Engineer
Izhevsk, Russia, www.mysql.com