Re: bzr commit into mysql-trunk branch (kristofer.pettersson:3197) Bug#29071

[Sergey Vojtovich]

Hi Kristofer,

On Wed, Sep 29, 2010 at 02:19:18PM +0200, Kristofer Pettersson wrote:
> 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.
Sure.

> > 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.
Ok. Can't disagree. I hope we're talking about my_stream_opened problem,
which is caused by incorrect handling of my_strdup() OOM. The same problem
is true for my_file_opened and I consider these problems minor. Minor
because much more serious problems are expected when server goes OOM.
Please note that I still consider this as a problem. But minor.

> > 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.
That was my hope.

> > 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?
IMHO refactoring is something that require an in-depth analysis of
subsystem, clean understanding of it's internals, understanding of
it's problems and improved design.

So far I didn't find answers for many question in cset comments.

I can't agree with the statement that the proposed fix is the only
logical grouping. We have slightly different logical grouping and
I see no problems with it.

In other words I don't feel I'm authorized to approve this refactoring
as such. I have a strong feeling that it is risky. Though if release
manager, e.g. Joro approves it, I would have no objections.

> > 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?
Not sure, some cases are explained below.

> > 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.
Sure, here they're (please correct me if I'm wrong):
- my_file_limit became hard limit for open files. For non-win this limit is
  64 by default. In other words applications are now forced to call
  my_set_max_open_files().

- in my_fclose(), call to my_win_close() (which is updating my_file_info) is
  not protected by THR_LOCK_open.

- did you check if the comment re fileno() still applies? I believe it is
  archaic, it has sense when the condition was like fileno(fd) > MY_NFILE.
  Now it seems useles.

- there are a couple of redundant checks introduced with this patch
  and our rule was always to avoid redundancy (e.g. my_fclose() checks
  if fd is not null).

More analysis when Joro approves this refactoring.
 
> > 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?
No. I mean: within the scope of this bug we should fix my_close() issue and
_optionally_ my_strdup() issue. The reasons are above in the text: I consider
my_strdup() issue minor.

P.S. I ran main, rpl, my_open-t (w/o modifications) and my_fopen-t (with a few
corrections) - all passed with my patch.

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