List:Commits« Previous MessageNext Message »
From:Kristofer Pettersson Date:October 1 2010 9:46am
Subject:Re: bzr commit into mysql-trunk branch (kristofer.pettersson:3197)
Bug#29071
View as plain text  
Sergey Vojtovich skrev 2010-10-01 10:36:
>>>> 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.
> 

Currently set_max_open_files() is called unconditionally from
init_common_variables() which in turn is called unconditionally from
main() in 5.1 release. In the trunk it is called from mysqld_main()
(which is still called win_main() in Windows for some bizarre reason).

In set_max_open_files() the only way to be certain how many max files we
allow is to check the return value. Despite this we still try to make an
educated guess by requesting max( max (wanted_files, max_connections*5),
open_files_limit) where wanted_files is based on obsolete MyISAM
requirements and open_files_limit is an externally declared variable (or
0 if the user didn't supply an open_files_limit).

Inside of set_max_open_files() we have the following magic:
  files+= MY_FILE_MIN;
  files= set_max_open_files(min(files, OS_FILE_LIMIT));
  if (files <= MY_NFILE)
    DBUG_RETURN(files);

As you can see MY_NFILE is treated as some kind of upper limit. You
claim it is an lower limit for my_file_limit but I'm not clear on how
you established this fact. In case of files<=MY_NFILE we don't update
my_file_limit at all.

Also note that my_set_max_open_files() belongs to the mysys library and
access data which belongs to my_open()/my_fopen() and hence can indeed
be considered part of the API interfacing that data.

Now, where does MY_NFILE come in? In the case of my_fopen()? Nowhere.
MY_NFILE is only mentioned and refelcted on in a really ad hoc comment.
And in my_open() it isn't visible at all. MY_NFILE is also wrongly
defined in my_global.h and not mysys.h which would be a more logical
place. It is used in my_file both as some kind of default value when
things go bad (and remember that this default value is based on MyISAM
requirements and has an almost ad hoc value in Windows) and as a strange
and ad hoc lower limit.

The only thing we need to assert however is that the file descriptor
maps to the file info array. Everything else is subject to pre- or
post-tuning, platform specific and should be moved to a higher layer
where it belongs.

What other applications you are referring to?

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

You were correct. my_win_fclose() calls
static void invalidate_fd(File fd)
{
  DBUG_ENTER("invalidate_fd");
  DBUG_ASSERT(fd >= MY_FILE_MIN && fd < (int)my_file_limit);
  my_file_info[fd].fhandle= 0;
  DBUG_VOID_RETURN;
}

which access the protected data structure. I propose that this is
violently bad and that this deadly spin of descendant functions should
be avoided. I'll move the "my_file_info[fd].fhandle= 0;" into my_open()
/ my_fopen() respectively.

Not holding THR_LOCK_open while performing a system calls seems like a
good idea to me if it can be avoided.

fixed.

> 
>>> - 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.
> 
I have no idea how you connect the comment. I used the old code, but I
agree that this is better:

  file= my_fileno(fd);
  if ((uint) file >= (uint) my_file_limit)
  {
    /* Function called with wrong parameters */
    DBUG_RETURN(EBADF);
  }

Fixed.

>>> - 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.
> 
Yes, in a call to std fclose() or fileno() using a NULL argument would
crash the server. I suppose this is a question which depends on how you
envision the my_close() functions to be used. I think we should allow
for multiple close and not crash on invalid arguments but rather treat
them with caution. However, I'm not going to be stubborn about this and
if you insist that we should crash on NULL values I can remove this check.

Done.

>>
>>>>> 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.
> 
Right. Lets work with it until we're both satisfied. I think it is
better because in the end at least two people will fully understand
what's going on.

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

I argue that we should allow for multiple closes and NULL arguments.
Copying the bare bone std calls doesn't make much sense. Especially not
when we must avoid crashing the server under all circumstances.



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