List:Commits« Previous MessageNext Message »
From:Kristofer Pettersson Date:September 30 2010 8:51pm
Subject:Re: bzr commit into mysql-trunk branch (kristofer.pettersson:3197)
Bug#29071
View as plain text  
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.

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.

> 
> - 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.

> 
> - 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.

> 
> - 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?

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.

>> 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.


> 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?

Regards,
Kristofer
Thread
bzr commit into mysql-trunk branch (kristofer.pettersson:3197) Bug#29071Kristofer Pettersson27 Sep
Re: bzr commit into mysql-trunk branch (kristofer.pettersson:3197)Bug#29071Kristofer Pettersson29 Sep
  • Re: bzr commit into mysql-trunk branch (kristofer.pettersson:3197)Bug#29071Sergey Vojtovich30 Sep
    • Re: bzr commit into mysql-trunk branch (kristofer.pettersson:3197)Bug#29071Kristofer Pettersson30 Sep
      • Re: bzr commit into mysql-trunk branch (kristofer.pettersson:3197)Bug#29071Sergey Vojtovich1 Oct
Re: bzr commit into mysql-trunk branch (kristofer.pettersson:3197)Bug#29071Sergey Vojtovich29 Sep
Re: bzr commit into mysql-trunk branch (kristofer.pettersson:3197)Bug#29071Kristofer Pettersson1 Oct
  • Re: bzr commit into mysql-trunk branch (kristofer.pettersson:3197)Bug#29071Sergey Vojtovich1 Oct
    • Re: bzr commit into mysql-trunk branch (kristofer.pettersson:3197)Bug#29071Kristofer Pettersson1 Oct
      • Re: bzr commit into mysql-trunk branch (kristofer.pettersson:3197)Bug#29071Sergey Vojtovich1 Oct
        • Re: bzr commit into mysql-trunk branch (kristofer.pettersson:3197)Bug#29071Kristofer Pettersson4 Oct