From: Sergei Golubchik Date: July 10 2009 2:51pm Subject: Re: GSoC I_S/P_S storage engine midterm report List-Archive: http://lists.mysql.com/soc/405 Message-Id: <20090710145133.GB3956@janus.mylan> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Hi, scut_tang! On Jul 10, scut_tang wrote: > Hi, Sergei. Thanks you very much for your midterm review. > > >>3.1 add support for I_S plugins. > What does "add support for I_S plugins" mean? "I_S plugins" are plugins that add new I_S table. see, for example, http://rpbouman.blogspot.com/2008/02/mysql-information-schema-plugins-best.html http://rpbouman.blogspot.com/2008/02/reporting-mysql-internals-with.html http://krow.net/talks/PluggageInformationSchemaVancouver2007.pdf don't worry too much about it now, but eventually your list of infoschema tables will need to be modifiable run-time. > >>3.2 make sure all SELECT queries work - for example, without position > >> and rnd_pos you cannot support filesort (ORDER BY), so there should > >> be some workaround for this case. > I knew position and rnd_pos are for non-sequential reads, and position > stores the current position of data file or something else about > current record, and rnd_pos reads record according by pos stored by > position. But how do they work for non-sequential reads? I want to > know more them in depth. > Do they connect record with its position and sort the records in MySQL > server? in filesort, for example, MySQL accumulates the list of pairs {sort key, position}, sorts this list by the 'sort key', and then retrieves rows by the stored positions. that is, for every row MySQL calls position(), copies in a special data structure this position and the sort key, and later it calls rnd_pos() with this value of position(). > >>select table_schema, table_name from information_schema.tables does > >>not show your tables - as expected, because you didn't implement > >>hton->find_files() method, see below. > I have finished infoschema_find_files now, and it works. > "SHOW DATABASES" can list INFOSCHEMA. great! > But "use INFOSHEMA" does not work, and what functions deal with "use > database-name"? Don't worry about it. You cannot really do it from the engine, MySQL verifies that there's a directory with the same name as the database name. For INFORMATION_SCHEMA there's a special exception in the code. We won't touch it, let's live with the fact that "use" doesn't work. When "INFOSCHEMA" will be renamed to INFORMATION_SCHEMA, "use" will start working. In case you're curious, the function is mysql_change_db() that calls check_db_dir_existence(). > >>SHOW CREATE TABLE INFOSCHEMA.TABLES crashes. not good. > I added "DBUG_RETURN(HA_ERR_COMMAND)" in ha_infoschema::create. > It won't crash. Does it show the correct table structure ? > >>Did you try both static and dynamic builds of your plugin ? > >>I mean, you declare here that your infoschema can be statically compiled > >>into the server or build as a shared library and loaded dynamically. > >>Are you sure that both work ? If you declare both ways, you > >>need to know that they both work. Otherwise, just keep the static one > >>and remove MYSQL_PLUGIN_DYNAMIC line if dynamic loading doesn't work. > >>I don't think that it makes sense to make infoschema dynamically loaded anyway, > >>so feel free to remove MYSQL_PLUGIN_DYNAMIC line. > I am sorry. I have only tried static builds of the plugin. > Why "I don't think that it makes sense to make infoschema dynamically loaded anyway"? I see it as an integral part of the server, not as a separate plugin. And it'll be using server internals (bypassing public API's) so it'll make little sense to load it run-time and version separately. > >>> + private: > >>> + /** MySQL lock */ > >>> + THR_LOCK_DATA m_thr_lock; > >>> + /** INFORMATION_SCHEMA table share for this handler. */ > >>> + const infos_table_share *m_table_share; > > >>you don't need it here, store it in TABLE_SHARE::ha_data > Why? For saving memory? I think it is just a pointer and it looks more clearly. okay. TABLE_SHARE::ha_data was specifically added to store storage engine "shared" data structures. If you want to store it here, fine, do it. But please also store it in the TABLE_SHARE::ha_data - because MySQL may open many handlers (many instances of the ha_infoschema class) for the same table, and the same TABLE_SHARE. Then in ::open you could do if (!table->s->ha_data) table->s->ha_data= find_table_share(table_share->db.str, ... m_table_share= table->s->ha_data; and you'll avoid going to the list of shares (which is a global array or a global hash, possibly protected with a mutex) in cases when the value is already known. > >>okay. why did you decide to have a separate hierarchy for the I_S tables, > >>deriving them from infos_table instead of using the handler hierarchy and > >>deriving them from ha_infoschema ? > > I have changed the hierarchy a little. > I will send the code diff file to you in this weekly report. Thanks! > >>are you sure you need #define MYSQL_SERVER here ? > I really do not understand "#define MYSQL_SERVER". Can you explain it? > I saw almost every storage engine contain it, so I do this. if you'll look into mysql_priv.h you'll see that with MYSQL_SERVER defined you'll see all internal server definitions, without it - only a subset of it (that we thought should be "fine" for storage engines). Try without MYSQL_SERVER - if it won't compile, then add it back but explain in a comment or email why do you need it (what declarations from mysql_priv.h you need to see, how the compilation fails without MYSQL_SERVER). > I am sorry for copied so much. My mind is it starts from a skeleton, > and remove useless functions in the development. No problem. For a first prototype it was excellent! Regards / Mit vielen Grüßen, Sergei -- __ ___ ___ ____ __ / |/ /_ __/ __/ __ \/ / Sergei Golubchik / /|_/ / // /\ \/ /_/ / /__ Principal Software Engineer/Server Architect /_/ /_/\_, /___/\___\_\___/ Sun Microsystems GmbH, HRB München 161028 <___/ Sonnenallee 1, 85551 Kirchheim-Heimstetten Geschäftsführer: Thomas Schroeder, Wolfgang Engels, Wolf Frenkel Vorsitzender des Aufsichtsrates: Martin Häring