Drizzle

Code review comment for lp:~posulliv/drizzle/info-schema-storage-engine

info-schema-storage-engine
Merge into development

Revision history for this message

Jay Pipes (jaypipes) wrote on 2009-10-19:

Hi Padraig!

A few comments and questions... :)

1) Per our IRC conversation, you should put back the ASSERT_COLUMN_MARKED_FOR_WRITE macros in /field/int64_t.cc and /field/long.cc.

2) What was the reasoning behind this change?

8 -#define PROCESS_LIST_INFO_WIDTH 65535
9 +#define PROCESS_LIST_INFO_WIDTH 16383

3) When I saw this removed from drizzled/join.cc:

92 - if ((this->select_lex->options & OPTION_SCHEMA_TABLE) && get_schema_tables_result(this, PROCESSED_BY_JOIN_EXEC))
93 - return;
94 -

and this removed from drizzled/sql_base.cc:

1070 - /*
1071 - If this TableList object is a placeholder for an information_schema
1072 - table, create a temporary table to represent the information_schema
1073 - table in the query. Do not fill it yet - will be filled during
1074 - execution.
1075 - */
1076 - if (tables->schema_table)
1077 - {
1078 - if (mysql_schema_table(this, lex, tables) == false)
1079 - continue;
1080 - return -1;
1081 - }

I had a little celebration :) No more two different code paths just for selecting from the information_schema \o/ epic win.

4) The following code needs to have the comment removed :)

237 + void addRow(unsigned char *buf, size_t len)
238 + {
239 + /*unsigned char *my_buf= new
240 + unsigned char[len];
241 + memcpy(my_buf, buf, len);
242 + rows.push_back(my_buf);*/
243 + InfoSchemaRecord tmp_rec(buf, len);
244 + rows.push_back(tmp_rec);
245 + }

Also, rows is defined as std::vector<InfoSchemaRecord>. Perhaps it is worth making this std::vector<InfoSchemaRecord *> instead and making the addRow() method this:

void addRow(unsigned char *buf, size_t len)
{
InfoSchemaRecord *record= new InfoSchemaRecord(buf, len);
rows.push_back(record);
}

and then cleaning up all those new'd records when the InfoSchemaTable is destroyed (in other words, have the InfoSchemaRecord manage all memory allocation instead of std::vector<>. This would be a little more efficient, as one less memcpy() would be used for each addRow() call.

5) I found this code to be awkward:

434 - error= test(schema_table->processTable(session, show_table_list,
435 - table, res, db_name,
436 + error= test(schema_table->processTable(schema_table,
437 + session,
438 + show_table_list,
439 + table,
440 + res,
441 + db_name,

Why is the first param of processTable() schema_table? I mean, "this" would refer to schema_table, no?

6) Indentation off by one here:

699 - XXX-> show_table_list has a flag i_is_requested,
700 - and when it's set, openTables()
701 - can return an error without setting an error message
702 - in Session, which is a hack. This is why we have to
703 - check for res, then for session->is_error() only then
704 - for session->main_da.sql_errno().
705 - */
706 + XXX-> show_table_list has a flag i_is_requested,
707 + and when it's set, openTables()
708 + can return an error without setting an error message
709 + in Session, which is a hack. This is why we have to
710 + check for res, then for session->is_error() only then
711 + for session->main_da.sql_errno().
712 + */

717 - Hide error for not existing table.
718 - This error can occur for example when we use
719 - where condition with db name and table name and this
720 - table does not exist.
721 - */
722 + Hide error for not existing table.
723 + This error can occur for example when we use
724 + where condition with db name and table name and this
725 + table does not exist.
726 + */

733 - We should use show_table_list->alias instead of
734 - show_table_list->table_name because table_name
735 - could be changed during opening of I_S tables. It's safe
736 - to use alias because alias contains original table name
737 - in this case.
738 - */
739 + We should use show_table_list->alias instead of
740 + show_table_list->table_name because table_name
741 + could be changed during opening of I_S tables. It's safe
742 + to use alias because alias contains original table name
743 + in this case.
744 + */

It's tough to tell in an editor, easy to tell on the diff output :)

7) Related to #5 above...

764 +int plugin::InfoSchemaMethods::processTable(
765 + plugin::InfoSchemaTable *store_table,
766 + Session *session, TableList *tables,

and then later in that method:

store_table->addRow(table->record[0], table->s->reclength);

Why is there a store_table argument? I understand that the code is a bit legacy, but I'm wondering why this code is like this? Is processTable() ever called when "this" is not the store_table argument? I can find no cases in the source code where this is the case. ack-grep turns up the following places where processTable() is called:

drizzled/plugin/info_schema_table.h
341: int retval= i_s_methods->processTable(store_table, session, tables, table,

drizzled/show.cc
1554: error= test(schema_table->processTable(schema_table,
1810: res= schema_table->processTable(schema_table,

It's clear that the calls to processTable() in drizzled/show.cc are passing "this" as the store_table parameter. For the call to processTable() in drizzled/plugin/info_schema_table.h, I investigated and I found this:

  int processTable(InfoSchemaTable *store_table,
                   Session *session, TableList *tables, Table *table,
                   bool res, LEX_STRING *db_name, LEX_STRING *tab_name) const
  {
    int retval= i_s_methods->processTable(store_table, session, tables, table,
                                          res, db_name, tab_name);
    return retval;
  }

I think that the store_table parameter of InfoSchemaTables::processTable() can be safely removed and the above can be changed to this:

  int processTable(Session *session, TableList *tables, Table *table,
                   bool res, LEX_STRING *db_name, LEX_STRING *tab_name) const
  {
    int retval= i_s_methods->processTable(this, session, tables, table,
                                          res, db_name, tab_name);
    return retval;
  }

Make sense?

8) Could you rework the below so that the assignment is done on a separate line from the comparison? It would make it clearer for old men like me to read ;)

1131 + if (! (need_start_waiting= ! wait_if_global_read_lock(session, 0, 1)))
1132 + return true;

9) Eventually, I'd love to get rid of the TableList::schema_table member altogether. After all, with this storage engine, an I_S table is just the same as any other table. Not that you should do this in this commit, but for future work... :)

10) Another something-to-do-later suggestion...

In InfoSchemaEngine::getTableProtoImplementation() I see some suspect constants:

2244 + if (column->getFlags() & MY_I_S_MAYBE_NULL)
2250 + if (column->getFlags() & MY_I_S_UNSIGNED)

I'm wondering if we can get rid of those flags from drizzled/definitions.h and instead use these from drizzled/common.h:

#define NOT_NULL_FLAG 1 /* Field can't be NULL */
#define UNSIGNED_FLAG 32 /* Field is unsigned */

The values of the flags are different, but it would be good to get rid of those I_S-specific #defines in drizzled/definitions.h that are inconsistent with the flags in common.h that are used in the rest of the code base.

11) krow has also been looking at a storage engine for I_S and has another branch with some work in it. I'd like you and krow to get together and discuss your branch :)

Cheers!

Jay

Hi Padraig!

A few comments and questions... :)

1) Per our IRC conversation, you should put back the ASSERT_COLUMN_MARKED_FOR_WRITE macros in /field/int64_t.cc and /field/long.cc.

2) What was the reasoning behind this change?

8	-#define PROCESS_LIST_INFO_WIDTH 65535
9	+#define PROCESS_LIST_INFO_WIDTH 16383

3) When I saw this removed from drizzled/join.cc:

92	- if ((this->select_lex->options & OPTION_SCHEMA_TABLE) && get_schema_tables_result(this, PROCESSED_BY_JOIN_EXEC))
93	- return;
94	-

and this removed from drizzled/sql_base.cc:

1070	- /*
1071	- If this TableList object is a placeholder for an information_schema
1072	- table, create a temporary table to represent the information_schema
1073	- table in the query. Do not fill it yet - will be filled during
1074	- execution.
1075	- */
1076	- if (tables->schema_table)
1077	- {
1078	- if (mysql_schema_table(this, lex, tables) == false)
1079	- continue;
1080	- return -1;
1081	- }

I had a little celebration :) No more two different code paths just for selecting from the information_schema \o/ epic win.

4) The following code needs to have the comment removed :)

237	+ void addRow(unsigned char *buf, size_t len)
238	+ {
239	+ /*unsigned char *my_buf= new
240	+ unsigned char[len];
241	+ memcpy(my_buf, buf, len);
242	+ rows.push_back(my_buf);*/
243	+ InfoSchemaRecord tmp_rec(buf, len);
244	+ rows.push_back(tmp_rec);
245	+ }

Also, rows is defined as std::vector<InfoSchemaRecord>.  Perhaps it is worth making this std::vector<InfoSchemaRecord *> instead and making the addRow() method this:

void addRow(unsigned char *buf, size_t len)
{
  InfoSchemaRecord *record= new InfoSchemaRecord(buf, len);
  rows.push_back(record);
}

and then cleaning up all those new'd records when the InfoSchemaTable is destroyed (in other words, have the InfoSchemaRecord manage all memory allocation instead of std::vector<>.  This would be a little more efficient, as one less memcpy() would be used for each addRow() call.

5) I found this code to be awkward:

434	- error= test(schema_table->processTable(session, show_table_list,
435	- table, res, db_name,
436	+ error= test(schema_table->processTable(schema_table,
437	+ session,
438	+ show_table_list,
439	+ table,
440	+ res,
441	+ db_name,

Why is the first param of processTable() schema_table?  I mean, "this" would refer to schema_table, no?

6) Indentation off by one here:

699	- XXX-> show_table_list has a flag i_is_requested,
700	- and when it's set, openTables()
701	- can return an error without setting an error message
702	- in Session, which is a hack. This is why we have to
703	- check for res, then for session->is_error() only then
704	- for session->main_da.sql_errno().
705	- */
706	+ XXX-> show_table_list has a flag i_is_requested,
707	+ and when it's set, openTables()
708	+ can return an error without setting an error message
709	+ in Session, which is a hack. This is why we have to
710	+ check for res, then for session->is_error() only then
711	+ for session->main_da.sql_errno().
712	+ */

717	- Hide error for not existing table.
718	- This error can occur for example when we use
719	- where condition with db name and table name and this
720	- table does not exist.
721	- */
722	+ Hide error for not existing table.
723	+ This error can occur for example when we use
724	+ where condition with db name and table name and this
725	+ table does not exist.
726	+ */

733	- We should use show_table_list->alias instead of
734	- show_table_list->table_name because table_name
735	- could be changed during opening of I_S tables. It's safe
736	- to use alias because alias contains original table name
737	- in this case.
738	- */
739	+ We should use show_table_list->alias instead of
740	+ show_table_list->table_name because table_name
741	+ could be changed during opening of I_S tables. It's safe
742	+ to use alias because alias contains original table name
743	+ in this case.
744	+ */

It's tough to tell in an editor, easy to tell on the diff output :)

7) Related to #5 above...

764	+int plugin::InfoSchemaMethods::processTable(
765	+ plugin::InfoSchemaTable *store_table,
766	+ Session *session, TableList *tables,

and then later in that method:

store_table->addRow(table->record[0], table->s->reclength);

Why is there a store_table argument?  I understand that the code is a bit legacy, but I'm wondering why this code is like this?  Is processTable() ever called when "this" is not the store_table argument?  I can find no cases in the source code where this is the case.  ack-grep turns up the following places where processTable() is called:

drizzled/plugin/info_schema_table.h
341:    int retval= i_s_methods->processTable(store_table, session, tables, table,

drizzled/show.cc
1554:   error= test(schema_table->processTable(schema_table,
1810:            res= schema_table->processTable(schema_table,

It's clear that the calls to processTable() in drizzled/show.cc are passing "this" as the store_table parameter.  For the call to processTable() in drizzled/plugin/info_schema_table.h, I investigated and I found this:

I think that the store_table parameter of InfoSchemaTables::processTable() can be safely removed and the above can be changed to this:

Make sense?

8) Could you rework the below so that the assignment is done on a separate line from the comparison?  It would make it clearer for old men like me to read ;)

1131	+ if (! (need_start_waiting= ! wait_if_global_read_lock(session, 0, 1)))
1132	+ return true;

9) Eventually, I'd love to get rid of the TableList::schema_table member altogether.  After all, with this storage engine, an I_S table is just the same as any other table.  Not that you should do this in this commit, but for future work... :)

10) Another something-to-do-later suggestion...

In InfoSchemaEngine::getTableProtoImplementation() I see some suspect constants:

2244	+ if (column->getFlags() & MY_I_S_MAYBE_NULL)
2250	+ if (column->getFlags() & MY_I_S_UNSIGNED)

I'm wondering if we can get rid of those flags from drizzled/definitions.h and instead use these from drizzled/common.h:

#define NOT_NULL_FLAG	1		/* Field can't be NULL */
#define UNSIGNED_FLAG	32		/* Field is unsigned */

11) krow has also been looking at a storage engine for I_S and has another branch with some work in it.  I'd like you and krow to get together and discuss your branch :)

Cheers!

Jay

« Back to merge proposal