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

[Sergey Vojtovich]

Hi Kristofer,

On Thu, Sep 30, 2010 at 10:51:20PM +0200, Kristofer Pettersson wrote:
> Sergey Vojtovich skrev 2010-09-30 18:25:
> 
> >> 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().
> MY_NFILE is defined as 64 but this value was never used since it is
> smaller than the lowest possible limit for a default configuration. My
> first mistake when making this patch was to use MY_NFILE as an upper
> limit and this caused several test files to break.
You say that MY_NFILE is never used, but I see it serves two purposes:
default value for my_file_limit, min value for my_file_limit.

> From what I can tell my_set_max_open_files() is always called before the
> server becomes operational and I think this is now to be considered part
> of an IO API.
For mysys library users, there is probably a requirement to call my_init(),
but I would be very surprised if my_set_max_open_files() were another
requirement for applications using IO. I wish sane default here.

> > - in my_fclose(), call to my_win_close() (which is updating my_file_info) is
> >   not protected by THR_LOCK_open.
> 
> I agree that this is a weak point in my patch which I haven't verified.
> I assumed that my_win_fclose() was an equivalent to fclose() and as such
> won't contain any data that should be protected by THR_LOCK_open. In
> fact if it does (I will check) then we should probably try to change that.
Ok.

> > - 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.
> Are you referring to how fileno works for old SUN machines? I left it as
> a TODO when we can prove that this is still an issue. I agree that the
> comment seems archaic.
Revision history is a good proof. This comment justifies type cast:
char fileno() returns 10
MY_FILENO is 200

(char) 10 is larger than (char) 200
(uint) 10 is smaller than (uint) 200

Now we compare int vs uint. Comment is obsolete.

> > - 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).
> I think it is good that a function validates its parameters. Where is
> the redundancy? How would you like it to be?
Talking particulary about my_fclose() check. Why would one want to call
my_fclose(NULL)? I'd rather expect broken or closed FILE structure, but
you cannot validate it in this case.

> I agree that we should write efficient code although I'm not sure what
> rule you are refering to.
>
> > More analysis when Joro approves this refactoring.
> > 
> ok
> 
> >>> 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 know you do. I would how ever not be very satisfied with a yet another
> patch on this already heavy patchwork. Better to get a clean slate and
> create something to be proud of.
Ok, I believe I wasn't clear enough, my point is: I find that the result
of this refactoring is different code. Not worse, not better - just
different.

> >> 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.
> 
> I don't think the bug scope is all that static. I don't think the triage
> team envisioned that the problem was in my_close() that there might be
> other issues as well. I rather think that everybody counts on us to step
> up and take responsibility for the code ownership and do what we believe
> in and make sure we support that belief with solid test cases.
Sure. And this is a good example when beliefs are different. We need third
to proceed.

> > 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.
> > 
> Which corrections did you make to the test case?
One was correction: avoid my_fclose(NULL);
Second was fix: avoid multiple my_fclose() on the same file descriptor (at least
not permitted according to fclose manual).

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