List:Commits« Previous MessageNext Message »
From:Kristofer Pettersson Date:October 1 2010 12:46pm
Subject:Re: bzr commit into mysql-trunk branch (kristofer.pettersson:3197)
Bug#29071
View as plain text  
Sergey Vojtovich skrev 2010-10-01 13:17:
> 
>> 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.
> Hmm...
> Do I interpret the last statement correctly: "my_file_limit cannot be larger
> than MY_NFILE"?

No. I'm rather questioning how you can be so certain about intention and
actual implementation given the strangeness of the if-statements and the
spaghetti like references to global variables and definitions. There
isn't a trivial mapping between my_file_limit and MY_NFILE as far as I
can tell. Nor does it really bother me, but maybe it should?

> 
> My statement was: "my_file_limit cannot be lower than MY_NFILE".
> 

I don't know for sure if this is right according to the implementation
and I don't know if this restrain is the original intention either. Let
say that you are right.

Does this check belong in the mysys layer anyway? I think it belongs in
mysqld.cc if anywhere.

I believe that my_open/my_fopen functions need to assert is that a file
descriptor is correctly mapped on the file info array.

No other logic is needed in this layer. The intention of the my_open
wrapping is two folded:
  1. Associate a position in the file info array to an open file
  2. Encapsulate platform specific code and presents a portable layer to
the user.

>> 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.
> Yes, it is part of the API.
ok, we agree.

> 
>> 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.
> mysys/my_static.c:struct st_my_file_info my_file_info_default[MY_NFILE];
> mysys/my_static.c:uint   my_file_limit= MY_NFILE;

Right, those too. I dare say that the default structure isn't used most
of the time though since a new allocations are likely to happen in
my_set_max_open_files(). It is however possible it is used as a default
structure if something goes bad like OOM. The MY_NFILE value is still
based on obsolete MyISAM requirements anyway or on ad hoc Windows
requirements.


> 
>> 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?
> Let's see...
> mysqladmin, mysqlbinlog, mysql, mysqlslap, mysqltest, myisamlog, myisamchk,
> archive_reader.

I think these application can survive this refactoring and in this case
it is easy to see that the server is more important.

> Abstract applications linked against libmysqlclient.
It is very unfortunate if people mistake the mysys library for being a
useful portable platform to rely on. Anyway, it is impossible to
anticipate every possible use of our code. If we want to make any chage
(even "minimal" fixes for bugs) we must be guided by vision and a strong
sense of code ownership and responsibility. In fact, exactly what we're
showing now.

> Abstract applications linked against mysys.
I doubt that there are any significant projects which rely on mysys in a
way which will be affected by this refactoring. In anyway, the server is
the primary target for mysys otherwise we should extract mysys and run
it as a separate project.

> What about this case:
> An application is linked against mysql client library.
Yes, what issues do you anticipate? It would be great if you can
enumerate the issues you can think of otherwise it isn't possible to
respond to your criticism in any other way than stop trying.

> An application heavily uses file descriptors, e.g. it has 1000 file descriptors
> open.
We will add test cases for very many file descriptors. In the test case
I aspire to open around OS_FILE_LIMIT+MY_NFILE+1024 files. I can do a
better job at anticipating exactly how many fd we can open but at least
my test case attempts to open as many files as the my_set_max_files()
allows. I think that test cases inspire confidence.


> An application requests LOAD DATA LOCAL INFILE.
> It fails, because mysql client library by default refuse to work with
> file descriptors > 64.

Do you mean it should fail (why?) or it will fail now? What is the exact
issue and how does it relate to the patch?

>>
>> Not holding THR_LOCK_open while performing a system calls seems like a
>> good idea to me if it can be avoided.
> Agree the purpose of THE_LOCK_open is to protect some variables, but not
> system calls. OTOH spreading winfile specific things around code is not
> a good idea either.
> 
>> fixed.

I replaced my_win_fopen() with direct invalidation of fhandle so
technically we lost one reference to my_win_file and got another
dependency on Windows cross platform. I think this is ok. I really
believe that using both my_win_file and my_open / my_fopen as a cross
platform encapsulation of the file info structure has a peculiar
redundancy to it and is also a layer violation. Don't you agree?


>>   file= my_fileno(fd);
>>   if ((uint) file >= (uint) my_file_limit)
>>   {
>>     /* Function called with wrong parameters */
>>     DBUG_RETURN(EBADF);
>>   }
>>
>> Fixed.
> I can always elaborate. What's wrong with my explanation?
I'll just assume nothing is wrong with your explanation. Are you
satisfied with the change above? ie using uint cast on both side?

>>>> 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.
> Would you do the same for other calls like read/write/fileno/etc?
> 
Good question. I think the difference between my_fileno and my_fopen is
that my_fileno just does platform encapsluation and my_fopen does file
info array associations and updates statistics. This makes the
implementation in its intention much more 'raw' and not really a new
abstraction layer given a specific platform.

For my_fread and my_fwrite I would definitely work towards making them
more crash safe by validating the parameters where it can be done before
sending them to a system call.

Cheers,
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