List:Commits« Previous MessageNext Message »
From:Sergey Vojtovich Date:October 1 2010 7:57pm
Subject:Re: bzr commit into mysql-trunk branch (kristofer.pettersson:3197)
Bug#29071
View as plain text  
Kristofer,

attached please find mtr test case. It is supposed to give empty
resultset.

With non-patched server I get empty resultset.
With patched server I observe three problems:
- myisampack cannot join 64 tables;
- mysqltest cannot cat_file when there are 64 connections open;
- libmysqlclient cannot "LOAD DATA LOCAL INFILE" when
  there are 64 connections open.

Unfortunately it is yet another good proof that this patch is risky.
Please let me know if that's not enough - I can analyze more applications.

Regards,
Sergey

On Fri, Oct 01, 2010 at 02:46:42PM +0200, Kristofer Pettersson wrote:
> 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
> 
> 
> 
> -- 
> MySQL Code Commits Mailing List
> For list archives: http://lists.mysql.com/commits
> To unsubscribe:    http://lists.mysql.com/commits?unsub=1
> 

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

--source include/not_windows.inc

let $MYSQLD_DATADIR= `select @@datadir`;
disable_query_log;
let $ntables= 64;
let $i= $ntables;
let $myisampack_arg=-j $MYSQLD_DATADIR/test/t0;
while ($i)
{
  eval CREATE TABLE t$i(a INT);
  let $myisampack_arg=$myisampack_arg $MYSQLD_DATADIR/test/t$i;
  dec $i;
}
FLUSH TABLES;
enable_query_log;

# myisampack
--exec $MYISAMPACK $myisampack_arg

disable_query_log;
let $i= $ntables;
while ($i)
{
  eval DROP TABLE t$i;
  dec $i;
}
disable_warnings;
DROP TABLE IF EXISTS t0;
enable_warnings;
enable_query_log;

let $i= $ntables;
while ($i)
{
  connect(con$i,localhost,root,,);
  dec $i;
}

disable_query_log;
CREATE TABLE t1(a INT);
# mysqltest
cat_file $MYSQLD_DATADIR/test/t1.MYD;

# libmysqlclient
eval LOAD DATA LOCAL INFILE '$MYSQLD_DATADIR/test/t1.MYD' INTO TABLE t1;
DROP TABLE t1;
enable_query_log;

let $i= $ntables;
while ($i)
{
  disconnect con$i;
  dec $i;
}

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