From: Kristofer Pettersson
Date: September 29 2010 12:19pm
Subject: Re: bzr commit into mysql-trunk branch (kristofer.pettersson:3197)
 Bug#29071
List-Archive: http://lists.mysql.com/commits/119409
Message-Id: <4CA32EC6.5080605@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 8bit

Hi Sergey!

Thanks for your efforts!

Sergey Vojtovich skrev 2010-09-29 13:23:

> I have strong doubts about this patch.
Lets work together to straighten out any uncertainty. My efforts has let
me to believe that what I present it the best cause of action. Help me
understand why your way is better.

> 
> First of all I tend to think that the true problem is not around
> my_strdup(filename), but rather with inconsistency in my_close():
> it is expected that my_close() is always called with valid file
> descriptor, thus my_file_opened is decremented unconditionally.
> But I have strong feeling that this requirement is not met, and
> it is one more problem as such.

I agree that the issues connected to this bug are concentrated to the
behaviour of my_close() as I wrote in the bug report analysis. However,
we also have a problem with my_stream_opened as was demonstrated with
the test cases.

> 
> Here are a few relevant memories that came to my mind: probably
> there was an assertion that my_close() gets valid file descriptor;
> probably there was a bug in replication, which used to close random
> file descriptors; probably there was a bug in myisam, which used to
> call my_close(-1).

I think your memories reflect good light on the history and why things
became bad.

> 
> 
> Secondly, my_file_opened/my_stream_opened were bound to physically
> open files/streams. With this patch these counters are bound to files
> which are successfully "registered" in my_file_info[] array. This is
> probably a noble action, but I see no good reasons. We can fix (not
> workaround, but truely fix) the problem with much lesser effort.
> 

There is always a good reason to perform noble deeds. ;-)
I think my patch cleans up the code and groups the variables together in
what can be the only logical grouping. There might be ways to do a
"minimal patch" in some sense, but I think a proper refactoring is much
easier to take responsibility for not only the change set but also for
the entire functionality. How can say that they do that today anyway? I
also find it especially comforting that I can back it up with unit
testing. Did you find it lacking in some degree? Can I improve on it?


> 
> Thirdly, though open/close functionality seem to be trivial, it has
> rather a tricky logics - lots of things needs to be taken into account.

Are there rules which you think we should add to the test cases?

> I can already see a couple of problems with modified code, which makes me
> worried. This code is basis for many things and is around for ages. I'd
> prefer to avoid major modifications here, especially in GA.

Please give an example so that I can capture this in the unit test.

> 
> 
> For this particular bug I'd prefer to see a one-line fix in my_close().
> I'm ok if you want to fix my_strdup() issue as well, but this is yet
> another one-line fixes.

I'm not really following your line of thought here. Do you suggest that
I should open a second bug report?

Cheers,

Kristofer