List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:May 9 2011 8:31pm
Subject:Re: bzr commit into mysql-trunk branch (gleb.shchepa:3358) WL#4897
View as plain text  
Hello Gleb,

Good, good job.
The patch is well commented, I find the design (classes) easy to figure 
out. This has thus been a shorter review than expected :-)

Gleb Shchepa a écrit, Le 01.04.2011 15:54:
> #At file:///mnt/sda7/work/mysql-next-mr-opt-backporting-wl4897/ based on
> revid:tor.didriksen@stripped
> 
>  3358 Gleb Shchepa	2011-04-01
>       WL#4897: Add EXPLAIN INSERT/UPDATE/DELETE
>       
>       WL#4897 implements EXPLAIN command for INSERT, REPLACE and
>       single- and multi-table UPDATE and DELETE queries.
>      @ mysql-test/include/explain_non_select.inc
>         Coverage and regresstion tests and sanity checks for WL#4897.

GB5 s/regresstion/regression

>      @ mysql-test/r/explain_non_select.result
>         Coverage and regresstion tests and sanity checks for WL#4897.

GB6 same

>      @ mysql-test/t/explain_non_select.test
>         Coverage and regresstion tests and sanity checks for WL#4897.

GB7 same

>      @ sql/opt_explain.cc
>         WL#4897: Add EXPLAIN INSERT/UPDATE/DELETE

> === added file 'mysql-test/include/explain_non_select.inc'
> --- a/mysql-test/include/explain_non_select.inc	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/include/explain_non_select.inc	2011-04-01 13:53:57 +0000
> @@ -0,0 +1,63 @@
> +--echo #
> +--echo # query:  $query
> +--echo # select: $select
> +--echo #
> +
> +if ($select) {
> +--disable_query_log
> +--eval $select INTO OUTFILE '$MYSQLTEST_VARDIR/tmp/before_explain.txt'
> +--enable_query_log
> +}
> +
> +--eval EXPLAIN $query
> +if (`SELECT ROW_COUNT() > 0`) {
> +--echo # Erroneous query: EXPLAIN $query
> +--die Unexpected ROW_COUNT() <> 0
> +}
> +
> +--eval EXPLAIN EXTENDED $query
> +if (`SELECT ROW_COUNT() > 0`) {
> +--echo # Erroneous query: EXPLAIN EXTENDED $query
> +--die Unexpected ROW_COUNT() <> 0
> +}
> +
> +if ($select) {
> +--eval EXPLAIN EXTENDED $select
> +}
> +

GB8 I suggest to add some FLUSH TABLES and SHOW STATUS (like you did 
below), to show that "EXPLAIN DELETE/UPDATE" does not generate more 
"handler_read/write" calls than "EXPLAIN SELECT".
This will prove that EXPLAIN DELETE/UPDATE is not guilty of reading the 
tables' data more too much (I already know it's true, but better put it 
in the test).

> === added file 'mysql-test/r/explain_non_select.result'
> +DROP TABLE t1;
> +CREATE TABLE t1 (a INT);
> +INSERT INTO t1 VALUES (1), (2), (3);
> +#
> +# query:  DELETE   FROM t1 USING t1 WHERE a = 1

GB9 what does "USING t1" mean here (there's no join)?


> +#
> +# query:  DELETE   FROM t1 WHERE (@a:= a) ORDER BY a LIMIT 1
> +# select: SELECT * FROM t1 WHERE (@a:= a) ORDER BY a LIMIT 1
> +#
> +EXPLAIN DELETE   FROM t1 WHERE (@a:= a) ORDER BY a LIMIT 1;
> +id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
> +1	SIMPLE	t1	ALL	PRIMARY	PRIMARY	4	NULL	4	Using where
> +EXPLAIN EXTENDED DELETE   FROM t1 WHERE (@a:= a) ORDER BY a LIMIT 1;
> +id	select_type	table	type	possible_keys	key	key_len	ref	rows	filtered	Extra
> +1	SIMPLE	t1	ALL	PRIMARY	PRIMARY	4	NULL	4	100.00	Using where
> +EXPLAIN EXTENDED SELECT * FROM t1 WHERE (@a:= a) ORDER BY a LIMIT 1;
> +id	select_type	table	type	possible_keys	key	key_len	ref	rows	filtered	Extra
> +1	SIMPLE	t1	index	NULL	PRIMARY	4	NULL	1	400.00	Using where; Using index

GB10 400.0 is surprising.
The manual says "The filtered column indicates an estimated percentage 
of table rows that will be filtered by the table condition", we could 
imagine it should be <=100. I never saw values greater than 100 until now.
This isn't due to your patch, but I suggest filing a bug report. There's
another case below (search for 352040 below).

> +# query:  UPDATE t1 SET a = 10 WHERE a IN (SELECT a FROM t2)
> +# select: SELECT * FROM t1     WHERE a IN (SELECT a FROM t2)
> +#
> +EXPLAIN UPDATE t1 SET a = 10 WHERE a IN (SELECT a FROM t2);
> +id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
> +1	PRIMARY	t1	ALL	NULL	NULL	NULL	NULL	3	Using where
> +2	DEPENDENT SUBQUERY	t2	ALL	NULL	NULL	NULL	NULL	3	Using where
> +EXPLAIN EXTENDED UPDATE t1 SET a = 10 WHERE a IN (SELECT a FROM t2);
> +id	select_type	table	type	possible_keys	key	key_len	ref	rows	filtered	Extra
> +1	PRIMARY	t1	ALL	NULL	NULL	NULL	NULL	3	100.00	Using where
> +2	DEPENDENT SUBQUERY	t2	ALL	NULL	NULL	NULL	NULL	3	100.00	Using where
> +EXPLAIN EXTENDED SELECT * FROM t1     WHERE a IN (SELECT a FROM t2);
> +id	select_type	table	type	possible_keys	key	key_len	ref	rows	filtered	Extra
> +1	PRIMARY	t1	ALL	NULL	NULL	NULL	NULL	3	100.00	
> +1	PRIMARY	t2	ALL	NULL	NULL	NULL	NULL	3	100.00	Using where; FirstMatch(t1); Using
> join buffer (BNL, incremental buffers)

GB11 I wondered why semijoin is not used for UPDATE above; debugging 
shows it's intentional, per point "6" of resolve_subquery().

> +CREATE TABLE t1 (i INT, j INT);
> +INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 3), (4, 4), (5, 5);
> +#
> +# query:  DELETE FROM t1
> +# select: SELECT * FROM t1
> +#
> +EXPLAIN DELETE FROM t1;
> +id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
> +1	SIMPLE	NULL	NULL	NULL	NULL	NULL	NULL	5	Using delete_all_rows

GB12 it would be good to test with an innodb table too, to show that
EXPLAIN properly reports that innodb doesn't use "delete_all_rows". See
also my comment about sql_delete.cc.

> +# query:  DELETE   FROM t2 WHERE b = 10 ORDER BY a, c LIMIT 5
> +# select: SELECT * FROM t2 WHERE b = 10 ORDER BY a, c LIMIT 5
> +#
> +EXPLAIN DELETE   FROM t2 WHERE b = 10 ORDER BY a, c LIMIT 5;
> +id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
> +1	SIMPLE	t2	ALL	a	a	15	NULL	17602	Using where
> +EXPLAIN EXTENDED DELETE   FROM t2 WHERE b = 10 ORDER BY a, c LIMIT 5;
> +id	select_type	table	type	possible_keys	key	key_len	ref	rows	filtered	Extra
> +1	SIMPLE	t2	ALL	a	a	15	NULL	17602	100.00	Using where
> +EXPLAIN EXTENDED SELECT * FROM t2 WHERE b = 10 ORDER BY a, c LIMIT 5;
> +id	select_type	table	type	possible_keys	key	key_len	ref	rows	filtered	Extra
> +1	SIMPLE	t2	index	NULL	a	15	NULL	5	352040.00	Using where

GB13 352040.0 is fishy

> +#
> +# query:  DELETE   FROM t2 WHERE b = 10 ORDER BY a, c LIMIT 5
> +# select: SELECT * FROM t2 WHERE b = 10 ORDER BY a, c LIMIT 5
> +#
> +EXPLAIN DELETE   FROM t2 WHERE b = 10 ORDER BY a, c LIMIT 5;
> +id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
> +1	SIMPLE	t2	ALL	a	NULL	NULL	NULL	26	Using where; Using filesort
> +EXPLAIN EXTENDED DELETE   FROM t2 WHERE b = 10 ORDER BY a, c LIMIT 5;
> +id	select_type	table	type	possible_keys	key	key_len	ref	rows	filtered	Extra
> +1	SIMPLE	t2	ALL	a	NULL	NULL	NULL	26	100.00	Using where; Using filesort
> +Warnings:
> +Warning	1713	Cannot use range access on index 'a' due to type or collation
> conversion on field 'b'
> +EXPLAIN EXTENDED SELECT * FROM t2 WHERE b = 10 ORDER BY a, c LIMIT 5;
> +id	select_type	table	type	possible_keys	key	key_len	ref	rows	filtered	Extra
> +1	SIMPLE	t2	ALL	NULL	NULL	NULL	NULL	26	100.00	Using where; Using filesort
> +Warnings:
> +Note	1003	select `test`.`t2`.`a` AS `a`,`test`.`t2`.`b` AS `b`,`test`.`t2`.`c` AS
> `c`,`test`.`t2`.`d` AS `d` from `test`.`t2` where (`test`.`t2`.`b` = 10) order by
> `test`.`t2`.`a`,`test`.`t2`.`c` limit 5

GB14 here DELETE has warning 1713 but SELECT doesn't. It's 
understandable, as DELETE considered "a" (in "possible_keys"), then 
realized it was not usable; whereas SELECT didn't consider "a" so didn't 
even come to seeing the type/collation problem. However I don't know why 
DELETE considered index "a" and SELECT didn't...

> === added file 'sql/opt_explain.cc'
> --- a/sql/opt_explain.cc	1970-01-01 00:00:00 +0000
> +++ b/sql/opt_explain.cc	2011-04-01 13:53:57 +0000
> @@ -0,0 +1,1450 @@
> +/* Copyright (c) 2011, Oracle and/or its affiliates. All rights reserved.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; version 2 of the License.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program; if not, write to the Free Software Foundation,
> +   51 Franklin Street, Suite 500, Boston, MA 02110-1335 USA */
> +
> +#include "opt_explain.h"
> +#include "sql_partition.h" // for make_used_partitions_str()
> +
> +
> +/**
> +  A base for all Explain_* classes
> +
> +  This class hierarchy is a successor of the old select_describe() function
> +  implementation. It extends old select_describe() functionality to deal with
> +  single-table data-modifying commands (UPDATE and DELETE).

GB15 and INSERT/REPLACE VALUES() too?

GB16 in the final push, I guess you will delete references to old code
(like "old select_describe()"). Those mentions have helped me greatly to
understand the transition between trunk and your tree. But once your
tree is trunk, they will not be needed anymore...?

> +*/
> +
> +class Explain
> +{
> +private:
> +  List<Item> items; ///< item list to feed select_result::send_data()
> +  Item_null *nil; ///< pre-allocated NULL item to fill empty columns in EXPLAIN
> +
> +protected:
> +  /*
> +    Next "col_*" fields are intended for the filling by "explain_*()" methods.
> +    Then the make_list() method links these Items into "items" list.
> +
> +    NOTE: NULL value means that Item_null object will be pushed into "items"
> +          list instead.
> +  */
> +  Item_uint   *col_id; ///< "id" column: seq. number of SELECT withing the query

GB17 protected member variables (not member functions) are discouraged 
in the two C++ books I have (Stroustrup, Meyers). Stroustrup: "declaring 
data members protected is usually a design error".
But I don't have yet a good alternative to suggest. Maybe I will find one.

> +  Item_string *col_select_type; ///< "select_type" column
> +  Item_string *col_table_name; ///< "table" to which the row of output refers
> +  Item_string *col_partitions; ///< "partitions" column
> +  Item_string *col_join_type; ///< "type" column, see join_type_str array
> +  Item_string *col_possible_keys; ///< "possible_keys": comma-separated list
> +  Item_string *col_key; ///< "key" column: index that is actually decided to use
> +  Item_string *col_key_len; ///< "key_length" column: length of the "key" above
> +  Item_string *col_ref; ///< "ref":columns/constants which are compared to "key"
> +  Item_int    *col_rows; ///< "rows": estimated number of examined table rows
> +  Item_float  *col_filtered; ///< "filtered": % of rows filtered by condition
> +  Item_string *col_extra; ///< "extra" column: additional information
> +
> +  THD *thd; ///< cached THD pointer
> +  const CHARSET_INFO *cs; ///< cached pointer to system_charset_info
> +  JOIN *join; ///< top-level JOIN (if any) provided by caller
> +  SELECT_LEX *select_lex; ///< cached select_lex pointer
> +
> +  select_result *external_result; ///< result stream (if any) provided by caller
> +
> +public:
> +  explicit Explain(JOIN *join_arg= NULL)
> +  : nil(NULL),
> +    thd(current_thd),

GB18 I think that in all places where we create an Explain, we know the 
THD already, so can pass it, avoiding current_thd.

> +    cs(system_charset_info),
> +    join(join_arg),
> +    select_lex(join ? join->select_lex : &thd->lex->select_lex),
> +    external_result(join ? join->result : NULL)
> +  {
> +    init_columns();
> +  }
> +  virtual ~Explain() {}

GB19 Explain and Explain_table_base are only used as base classes in
practice, so I'd declare their constructors/destructors protected, to
prevent an accidental direct instantation.

> +
> +  bool send();
> +
> +private:
> +  void init_columns();
> +  bool make_list();

GB20 The doxygen comments for those two are in front of the
implementation; you are allowed to move them in front of the
declaration. I finds this helps the reader (he can understand the
"class' API" just by reading the class declaration). But it's up to you.

> +  bool push(Item *item) { return items.push_back(item ? item : nil); }
> +
> +protected:
> +  bool describe(uint8 mask) { return thd->lex->describe & mask; }
> +
> +  /**
> +    Prepare the self-allocated result object
> +
> +    For queries with top-level JOIN the caller provides pre-allocated
> +    select_send object. Then that JOIN object prepares the select_send
> +    object calling result->prepare() in JOIN::prepare(),
> +    result->initalize_tables() in JOIN::optimize() and result->prepare2()
> +    in JOIN::exe().

GB21 s/exe/exec

> +    However witout the presence of the top-level JOIN we have to

GB22 s/witout/without

> +    prepare/initialize select_send object manually.
> +  */
> +  bool prepare(select_result *result)
> +  {
> +    DBUG_ASSERT(join == NULL);
> +    List<Item> dummy;
> +    return result->prepare(dummy, select_lex->master_unit()) ||
> +           result->prepare2();
> +  }
> +
> +  virtual bool send_to(select_result *to);
> +
> +  /*
> +    Rest of the methods are overloadable functions those calculate and fill

GB23 missing comma?

> +    "col_*" fields with Items for further sending as EXPLAIN columns.
> +
> +    "explain_*" methods return FALSE on success and TRUE on error (usually OOM).
> +  */
> +  virtual bool explain_id();
> +  virtual bool explain_select_type();
> +  virtual bool explain_table_name() { return FALSE; }
> +  virtual bool explain_partitions() { return FALSE; }
> +  virtual bool explain_join_type() { return FALSE; }
> +  virtual bool explain_possible_keys() { return FALSE; }
> +  /* fill col_key and and col_key_len fields together */

GB24 /**

> +  virtual bool explain_key_and_len() { return FALSE; }
> +  virtual bool explain_ref() { return FALSE; }
> +  /* fill col_rows and col_filtered fields together */

GB25 /**

> +  virtual bool explain_rows_and_filtered() { return FALSE; }
> +  virtual bool explain_extra();
> +};
> +
> +
> +/**
> +  Explain_msg class outputs a trivial EXPLAIN row with "extra" column
> +
> +  Fromer part of the old select_describe() function


GB26 s/fromer/former.

GB27 in the final push, I guess you will delete references to old code?

> +  This class is intended for simple cases to produce EXPLAIN output
> +  with "No tables used", "No matching records" etc.
> +  Optionally it can output number of estimated rows in the "row"
> +  column.
> +
> +  NOTE: this class also produces EXPLAIN rows for inner units (if any).
> +*/
> +
> +class Explain_msg: public Explain
> +{
> +private:
> +  const char *message; ///< cached "message" argument
> +  const ha_rows rows; ///< HA_POS_ERROR or cached "rows" argument
> +
> +public:
> +  Explain_msg(JOIN *join_arg, const char *message_arg)

GB28 should this constructor be explicit? same question for other ctors 
in the file.

> +  : Explain(join_arg), message(message_arg), rows(HA_POS_ERROR)
> +  {}
> +  
> +  explicit Explain_msg(const char *message_arg, ha_rows rows_arg= HA_POS_ERROR)
> +  : message(message_arg), rows(rows_arg)
> +  {}
> +
> +protected:
> +  virtual bool explain_rows_and_filtered();
> +  virtual bool explain_extra();
> +};
> +
> +
> +/**
> +  Explain_union class outputs EXPLAIN row for UNION
> +
> +  Former part of the old select_describe() function.

GB29 in the final push, I guess you will delete references to old code?

> +*/
> +
> +class Explain_union : public Explain
> +{
> +private:
> +  char table_name_buffer[NAME_CHAR_LEN];
> +
> +public:
> +  Explain_union(JOIN *join_arg) : Explain(join_arg)
> +  {
> +    /* it's a UNION: */
> +    DBUG_ASSERT(join_arg->select_lex == join_arg->unit->fake_select_lex);
> +  }
> +
> +protected:
> +  virtual bool explain_id();
> +  virtual bool explain_table_name();
> +  virtual bool explain_join_type();
> +  virtual bool explain_extra();
> +};
> +
> +
> +
> +/**
> +  Common base class for Explain_join and Explain_table
> +
> +  Former part of the old select_describe() function.

GB30 in the final push, I guess you will delete references to old code?

> +*/
> +
> +class Explain_table_base : public Explain {
> +protected:
> +  TABLE *table;
> +  key_map usable_keys;
> +
> +  char buff_possible_keys[512];
> +  String str_possible_keys;

GB31 those two members are used only in
Explain_table_base::explain_possible_keys()
so I'd declare them as local variables of that function instead.
I don't imagine there will be a notable performance loss by doing that.
It's just allocating 512 bytes on the stack (incrementing the stack
pointer), and setting a few String members. And it keeps things as
localized as possible, which is good practice. Then they don't even need 
a Doxygen comment (as members variables, they would).

> +
> +public:
> +  explicit Explain_table_base(JOIN *join_arg)
> +  : Explain(join_arg), table(NULL),
> +    str_possible_keys(buff_possible_keys, sizeof(buff_possible_keys), cs)
> +  {}
> +
> +  explicit Explain_table_base(TABLE *table_arg)
> +  : table(table_arg),
> +    str_possible_keys(buff_possible_keys, sizeof(buff_possible_keys), cs)
> +  {}
> +
> +protected:
> +  virtual bool explain_partitions();
> +  virtual bool explain_possible_keys();
> +};
> +
> +
> +/**
> +  Explain_join class produces EXPLAIN output for JOINs
> +
> +  Former part of the old select_describe() function.


> +*/
> +
> +class Explain_join : public Explain_table_base
> +{
> +private:
> +  const bool need_tmp_table;
> +  const bool need_order;
> +  const bool distinct;
> +
> +  uint tabnum;
> +  JOIN_TAB *tab;

GB32 new members need a doxygen comment, like with ///<

> +  int quick_type;
> +  table_map used_tables;
> +  uint last_sjm_table;
> +
> +  char table_name_buffer[NAME_LEN];

GB33 I'd declare it local to Explain_join::explain_table_name(). Same
comment for Explain_union's table_name_buffer.

> +  char buff_key[512];
> +  String str_key;

GB34 same

> +  char buff_key_len[512];
> +  String str_key_len;

GB35 same

> +  char buff_ref[512];
> +  String str_ref;

GB36 same

> +  char buff_extra[512];
> +  String str_extra;

GB37 same

> +public:
> +  Explain_join(JOIN *join_arg,
> +               bool need_tmp_table_arg, bool need_order_arg,
> +               bool distinct_arg) 
> +  : Explain_table_base(join_arg), need_tmp_table(need_tmp_table_arg),
> +    need_order(need_order_arg), distinct(distinct_arg),
> +    tabnum(0), used_tables(0), last_sjm_table(MAX_TABLES),
> +    str_key(buff_key, sizeof(buff_key), cs),
> +    str_key_len(buff_key_len, sizeof(buff_key_len), cs),
> +    str_ref(buff_ref, sizeof(buff_ref), cs),
> +    str_extra(buff_extra, sizeof(buff_extra), cs)
> +  {
> +    /* it is not UNION: */
> +    DBUG_ASSERT(join_arg->select_lex != join_arg->unit->fake_select_lex);
> +  }
> +
> +protected:
> +  virtual bool send_to(select_result *to);
> +  virtual bool explain_table_name();
> +  virtual bool explain_join_type();
> +  virtual bool explain_key_and_len();
> +  virtual bool explain_ref();
> +  virtual bool explain_rows_and_filtered();
> +  virtual bool explain_extra();
> +};
> +
> +
> +/**
> +  Explain_table class produce EXPLAIN output for queries without top-level JOIN
> +
> +  This class is a simplified version of the Explain_join class. It works in the
> +  context of queries which implementation lacks top-level JOIN object (EXPLAIN
> +  single-table UPDATE and DELETE).
> +*/
> +
> +class Explain_table: public Explain_table_base
> +{
> +private:
> +  const SQL_SELECT *select;    ///< cached "select" argument
> +  const uint       key;        ///< cached "key" number argument
> +  const ha_rows    limit;      ///< HA_POS_ERROR or cached "limit" argument
> +  const bool       need_sort;  ///< cached need_sort argument
> +
> +  /*
> +    Pre-allocated buffers and String wrapper objects for them
> +  */
> +  char buff_key[512];
> +  String str_key;
> +  char buff_key_len[512];
> +  String str_key_len;
> +  char buff_extra[512];
> +  String str_extra;
> +
> +public:
> +  Explain_table(TABLE *table_arg, SQL_SELECT *select_arg,
> +                uint key_arg, ha_rows limit_arg, bool need_sort_arg)
> +  : Explain_table_base(table_arg), select(select_arg), key(key_arg),
> +    limit(limit_arg), need_sort(need_sort_arg),
> +    str_key(buff_key, sizeof(buff_key), cs),
> +    str_key_len(buff_key_len, sizeof(buff_key_len), cs),
> +    str_extra(buff_extra, sizeof(buff_extra), cs)
> +  {
> +    usable_keys= table->keys_in_use_for_query;
> +  }
> +  
> +private:
> +  virtual bool explain_table_name();
> +  virtual bool explain_join_type();
> +  virtual bool explain_key_and_len();
> +  virtual bool explain_rows_and_filtered();
> +  virtual bool explain_extra();
> +};
> +
> +
> +static join_type calc_join_type(int quick_type)
> +{
> +  if ((quick_type == QUICK_SELECT_I::QS_TYPE_INDEX_MERGE) ||
> +      (quick_type == QUICK_SELECT_I::QS_TYPE_ROR_INTERSECT) ||
> +      (quick_type == QUICK_SELECT_I::QS_TYPE_ROR_UNION))
> +    return JT_INDEX_MERGE;
> +  else
> +    return JT_RANGE;
> +}
> +
> +
> +/* Explain class methods ******************************************************/
> +
> +
> +/**
> +  Explain class main function
> +  

GB38 there is some trailing whitespace in the file

> +  This method:
> +    a) allocates a select_send object (if no one pre-allocated available),
> +    b) calculates and sends whole EXPLAIN data.
> +
> +  @returns
> +    @retval FALSE  Ok
> +    @retval TRUE   Error

GB39 nowadays the coding style allows true/false too, it's up to you.

> +*/
> +
> +bool Explain::send()
> +{
> +  /* Don't log this into the slow query log */
> +  thd->server_status&= ~(SERVER_QUERY_NO_INDEX_USED |
> +                         SERVER_QUERY_NO_GOOD_INDEX_USED);
> +
> +  select_result *result;
> +  if (external_result == NULL)
> +  {
> +    /* Create select_result object if the called doesn't provide one: */
> +    result= new select_send;
> +    if (!(result= new select_send))

GB40 two calls to "new", not what you wanted

> +      return TRUE;
> +    if (thd->send_explain_fields(result) || prepare(result))

GB41 it's not related to your patch, but I wonder why
send_explain_fields() is a member of THD and not a free function; it
uses only little of THD... Sending fields for a particular command is
not the job of THD, at least there isn't a similar member function for
the other SQL commands (or I missed it).

> +    {
> +      delete result;
> +      return TRUE;
> +    }
> +  }
> +  else
> +  {
> +    result= external_result;
> +    external_result->reset_offset_limit_cnt();

GB42 I commented out the call above and no test failed. Do you have an 
idea how to get coverage for it?

> +  }
> +
> +  if (!(nil= new Item_null))
> +    return TRUE;

GB43 Can send() be called twice? If it can, we don't have a real leak
above, because Item_int is created in a MEMROOT, not on the heap. But I
hope it would still not be called too many times for one statement?

> +  bool ret= send_to(result);
> +
> +  if (ret && join)
> +    join->error= 1;
> +
> +  for (SELECT_LEX_UNIT *unit= select_lex->first_inner_unit();
> +       unit && !ret;
> +       unit= unit->next_unit())
> +    ret= mysql_explain_union(thd, unit, result);
> +
> +  if (external_result == NULL)
> +  {
> +    if (ret)
> +      result->abort_result_set();
> +    else
> +      result->send_eof();
> +    delete result;
> +  }
> +  return ret;
> +}
> +
> +
> +/**
> +  Reset all "col_*" fields
> +*/
> +
> +void Explain::init_columns()
> +{
> +  col_id= NULL;
> +  col_select_type= NULL;
> +  col_table_name= NULL;
> +  col_partitions= NULL;
> +  col_join_type= NULL;
> +  col_possible_keys= NULL;
> +  col_key= NULL;
> +  col_key_len= NULL;
> +  col_ref= NULL;
> +  col_rows= NULL;
> +  col_filtered= NULL;
> +  col_extra= NULL;
> +}
> +
> +
> +/**
> +  Calculate EXPLAIN column values and link them into "items" list
> +
> +  @returns
> +    @retval FALSE  Ok
> +    @retval TRUE   Error
> +*/
> +
> +bool Explain::make_list()
> +{
> +  if (explain_id() ||
> +      explain_select_type() ||
> +      explain_table_name() ||
> +      explain_partitions() ||
> +      explain_join_type() ||
> +      explain_possible_keys() ||
> +      explain_key_and_len() ||
> +      explain_ref() ||
> +      explain_rows_and_filtered() ||
> +      explain_extra())
> +    return TRUE;
> +    
> +  /* 
> +    NOTE: the number/types of items pushed into item_list must be in sync with
> +    EXPLAIN column types as they're "defined" in THD::send_explain_fields()
> +  */
> +  return push(col_id) ||
> +         push(col_select_type) ||
> +         push(col_table_name) ||
> +         (describe(DESCRIBE_PARTITIONS) && push(col_partitions)) ||
> +         push(col_join_type) ||
> +         push(col_possible_keys) ||
> +         push(col_key) ||
> +         push(col_key_len) ||
> +         push(col_ref) ||
> +         push(col_rows) ||
> +         (describe(DESCRIBE_EXTENDED) && push(col_filtered)) ||
> +         push(col_extra);
> +}
> +
> +
> +/**
> +  Make "items" list and send it to select_result output stream
> +
> +  This method is virtual since the base implementation is intended for sending
> +  the "items" list once, but the overloaded Explain_join implementation sends
> +  it many times (once for each JOIN::join_tab[] element).
> +
> +  @returns
> +    @retval FALSE  Ok
> +    @retval TRUE   Error
> +*/
> +
> +bool Explain::send_to(select_result *to)
> +{
> +  bool ret= make_list() || to->send_data(items);

GB44 const bool

> +  items.empty();
> +  init_columns();
> +  return ret;
> +}
> +
> +
> +bool Explain::explain_id()
> +{
> +  col_id= new Item_uint(select_lex->select_number);
> +  return col_id == NULL;
> +}
> +
> +
> +bool Explain::explain_select_type()
> +{
> +  if (select_lex->type)
> +    col_select_type= new Item_string(select_lex->type,
> +                                     strlen(select_lex->type), cs);
> +  else if (select_lex->first_inner_unit() || select_lex->next_select())
> +    col_select_type= new Item_string(STRING_WITH_LEN("PRIMARY"), cs);
> +  else
> +    col_select_type= new Item_string(STRING_WITH_LEN("SIMPLE"), cs);
> +  return col_select_type == NULL;                                       
> +}
> +
> +
> +bool Explain::explain_extra()
> +{
> +  col_extra= new Item_string("", 0, cs);
> +  return col_extra == NULL;
> +}
> +
> +
> +/* Explain_msg class methods **************************************************/
> +
> +
> +bool Explain_msg::explain_rows_and_filtered()
> +{
> +  if (rows == HA_POS_ERROR)
> +    return FALSE;
> +  col_rows= new Item_int(rows, MY_INT64_NUM_DECIMAL_DIGITS);
> +  return col_rows == NULL;
> +}
> +
> +
> +bool Explain_msg::explain_extra()
> +{
> +  col_extra= new Item_string(message, strlen(message), cs);
> +  return col_extra == NULL;
> +}
> +
> +
> +/* Explain_union class methods ************************************************/
> +
> +
> +bool Explain_union::explain_id()
> +{
> +  col_id= NULL;

GB45 IIUC due to init_columns() col_id should already be NULL when we
enter the function?

> +  return FALSE;
> +}
> +
> +

> +bool Explain_union::explain_extra()
> +{
> +  /* 
> +    Moved from select_describe():

GB46 in the final push, I guess you will delete references to old code?

> +    here we assume that the query will return at least two rows, so we
> +    show "filesort" in EXPLAIN. Of course, sometimes we'll be wrong
> +    and no filesort will be actually done, but executing all selects in
> +    the UNION to provide precise EXPLAIN information will hardly be
> +    appreciated :)
> +  */
> +  if (join->unit->global_parameters->order_list.first)
> +  {
> +    col_extra= new Item_string(STRING_WITH_LEN("Using filesort"), cs);
> +    return col_extra == NULL;
> +  }
> +  return Explain::explain_extra();
> +}
> +
> +

> +bool Explain_join::explain_ref()
> +{
> +  str_ref.length(0);

GB47 can move the line above into "if (tab->ref.key_parts)". But (see 
other command) making it local would be even better.

> +  if (tab->ref.key_parts)
> +  {
> +    for (store_key **ref= tab->ref.key_copy; *ref; ref++)
> +    {
> +      if (str_ref.length())
> +        str_ref.append(',');
> +      str_ref.append((*ref)->name(), strlen((*ref)->name()), cs);
> +    }
> +    col_ref= new Item_string(str_ref.ptr(), str_ref.length(), cs);
> +    return col_ref == NULL;
> +  }
> +  return FALSE;
> +}
> +
> +
> +bool Explain_join::explain_rows_and_filtered()
> +{
> +  if (tab->table->pos_in_table_list->schema_table)

GB48 several 'tab->table' could be replaced with 'table'.

> +    return FALSE;
> +
> +  double examined_rows;
> +  if (tab->select && tab->select->quick)
> +    examined_rows= rows2double(tab->select->quick->records);
> +  else if (tab->type == JT_NEXT || tab->type == JT_ALL)
> +  {
> +    if (tab->limit)
> +      examined_rows= rows2double(tab->limit);
> +    else
> +    {
> +      tab->table->file->info(HA_STATUS_VARIABLE);
> +      examined_rows= rows2double(tab->table->file->stats.records);
> +    }
> +  }
> +  else
> +    examined_rows= join->best_positions[tabnum].records_read; 
> +
> +  col_rows= new Item_int((longlong) (ulonglong) examined_rows, 
> +                         MY_INT64_NUM_DECIMAL_DIGITS);

GB49 it's not your code but it's strange: Item_int has a constructor 
which accepts ulonglong, why do we cast to longlong here...

> +  if (col_rows == NULL)
> +    return TRUE;
> +
> +  /* Add "filtered" field */
> +  if (describe(DESCRIBE_EXTENDED))
> +  {
> +    float f= 0.0; 
> +    if (examined_rows)
> +      f= (float) (100.0 * join->best_positions[tabnum].records_read /
> +                  examined_rows);

GB50 not your code, but the cast to (float) looks weird, as 'f' is 
already a float.

> +    col_filtered= new Item_float(f, 2);
> +    if (col_filtered == NULL)
> +      return TRUE;

GB51 in Explain_join::explain_ref(), you do
return (col_xxx == NULL);
instead of the if().

> +  }
> +  return FALSE;
> +}
> +
> +
> +bool Explain_join::explain_extra()
> +{
> +  str_extra.length(0);
> +
> +  my_bool key_read=table->key_read;

GB52 space after =

> +  if ((tab->type == JT_NEXT || tab->type == JT_CONST) &&
> +      table->covering_keys.is_set(tab->index))
> +    key_read=1;

GB53 same. Please do a pass over this function to fix other little 
issues of this kind.

> +  if (quick_type == QUICK_SELECT_I::QS_TYPE_ROR_INTERSECT &&
> +     
> !((QUICK_ROR_INTERSECT_SELECT*)tab->select->quick)->need_to_fetch_row)
> +    key_read=1;

GB54 not your code, but can change "key_read" to bool, store true/false 
in it, and declare only closer to where it's used (which is an if() 
further down).

> +    if (distinct && test_all_bits(used_tables,thd->used_tables))
> +      str_extra.append(STRING_WITH_LEN("; Distinct"));
> +
> +    if (tab->loosescan_match_tab)
> +    {
> +      str_extra.append(STRING_WITH_LEN("; LooseScan"));
> +    }

GB55 can delete {}


> +bool Explain_table::explain_key_and_len()
> +{
> +  str_key.length(0);
> +  str_key_len.length(0);
> +
> +  if (key != MAX_KEY)
> +  {
> +    KEY *key_info= table->key_info + key;
> +    col_key= new Item_string(key_info->name, strlen(key_info->name), cs);
> +    int length= longlong2str(key_info->key_length, buff_key_len, 10) - 
> +                             buff_key_len;
> +    col_key_len= new Item_string(buff_key_len, length, cs);
> +    return col_key == NULL || col_key_len == NULL;
> +  }
> +  else if (select && select->quick)
> +  {
> +    select->quick->add_keys_and_lengths(&str_key, &str_key_len);
> +    col_key= new Item_string(str_key.ptr(), str_key.length(), cs);
> +    col_key_len= new Item_string(str_key_len.ptr(), str_key_len.length(), cs);
> +    return col_key == NULL || col_key_len == NULL;
> +  }
> +  return FALSE;

GB56 this is a copy-paste of half of 
Explain_join::explain_key_and_len(); is there a good way to avoid this?

> +}
> +
> +
> +bool Explain_table::explain_rows_and_filtered()
> +{
> +  double examined_rows;
> +  if (select && select->quick)
> +    examined_rows= rows2double(select->quick->records);
> +  else if (!select && !need_sort && limit != HA_POS_ERROR)
> +    examined_rows= rows2double(limit);
> +  else
> +  {
> +    table->file->info(HA_STATUS_VARIABLE);
> +    examined_rows= rows2double(table->file->stats.records);
> +  }
> +  col_rows= new Item_int((longlong) (ulonglong) examined_rows,
> +                         MY_INT64_NUM_DECIMAL_DIGITS);
> +  if (col_rows == NULL)
> +    return TRUE;
> +
> +  if (describe(DESCRIBE_EXTENDED))
> +  {
> +    col_filtered= new Item_float(100.0, 2);
> +    if (col_filtered == NULL)
> +      return TRUE;
> +  }

GB57 this is also partially a copy-paste of 
Explain_join::explain_rows_and_filtered().

> +  return FALSE;
> +}
> +
> +
> +bool Explain_table::explain_extra()
> +{

GB58 this one also looks like copy-paste of selected bits of 
Explain_join::extra(), is it avoidable?

> +  str_extra.length(0);
> +
> +  uint keyno= (select && select->quick) ? select->quick->index :
> key;
> +

> +class explain_send : public select_send {
> +protected:
> +  /**
> +    Bits for result_state bitmap
> +
> +    As far as we use explain_send object in a place of select_send, explain_send
> +    have to pass multiple invocation of its prepare(), prepare2() and
> +    initialize_tables() methods, since JOIN::exec() of subqueries runs
> +    these methods of select_send multiple times by design.
> +    insert_select, multi_update and multi_delete class methods are not intended
> +    for multiple invocations, so result_state bitmap guards data interceptor
> +    object from method re-invocation.
> +  */
> +  enum result_state_enum { 
> +    SELECT_RESULT_PREPARED    = 0x01, // 1st bit set: prepare() is complete
> +    SELECT_RESULT_PREPARED2   = 0x02, // 2nd bit set: prepare2() is complete
> +    SELECT_RESULT_INITIALIZED = 0x04  // 3rd bit set: initialize_tables() done

GB59 ///< instead of //. Can replace "complete" with "done", to be uniform.

> +  };
> +  int result_state; ///< bitmap of result_state_enum bits

GB60 "result_state" can be of type enum result_state_enum. Will be 
easier to read in gdb.

> +
> +  /**
> +    Pointer to underlying insert_select, multi_update or multi_delete object
> +  */
> +  select_result_interceptor *interceptor;
> +
> +public:
> +  explain_send(select_result_interceptor *interceptor_arg) 
> +  : result_state(0), interceptor(interceptor_arg)
> +  {}
> +
> +protected:
> +  virtual int prepare(List<Item> &list, SELECT_LEX_UNIT *u)
> +  {
> +    if (result_state & SELECT_RESULT_PREPARED)
> +      return FALSE;
> +    else
> +      result_state|= SELECT_RESULT_PREPARED;

GB61 can remove "else". If you prefer to keep it, then I suggest moving
the final return into the else block.

> +    return select_send::prepare(list, u) || interceptor->prepare(list, u);
> +  }
> +
> +  virtual int prepare2(void)
> +  {
> +    if (result_state & SELECT_RESULT_PREPARED2)
> +      return FALSE;
> +    else
> +      result_state|= SELECT_RESULT_PREPARED2;

GB62 same

> +    return select_send::prepare2() || interceptor->prepare2();
> +  }
> +
> +  virtual bool initialize_tables(JOIN *join)
> +  {
> +    if (result_state & SELECT_RESULT_INITIALIZED)
> +      return FALSE;
> +    else

GB63 same

> +      result_state|= SELECT_RESULT_INITIALIZED;
> +    return select_send::initialize_tables(join) ||
> +           interceptor->initialize_tables(join);
> +  }
> +
> +  virtual void cleanup()
> +  {
> +    select_send::cleanup();
> +    interceptor->cleanup();
> +  }
> +};
> +
> +
> +/******************************************************************************
> +  External function implementations
> +******************************************************************************/
> +
> +
> +/**
> +  Send a messages as an "extra" column value

GB64 s/messages/message

> +
> +  This function forms the 1st row of the QEP output with a simple text message.
> +  This is useful to explain such trivial cases as "No tables used" etc.
> +  
> +  NOTE: Also this function explains the rest of QEP (subqueries or joined
> +        tables if any).

GB65 NOTE -> @note (same remark for all NOTE in the patch).

> +  @param message  text message for the "extra" column.
> +  @param rows     HA_POS_ERROR or a value for the "rows" column.

GB66 params above do not correspond to real params

> +
> +  @returns
> +    @retval FALSE  OK
> +    @retval TRUE   Error
> +*/
> +
> +bool msg_describe(JOIN *join, const char *message)
> +{
> +  return Explain_msg(join, message).send();
> +}
> +
> +
> +/**
> +  Send a messages as an "extra" column value

GB67 s/messages/message

> +  This function forms the 1st row of the QEP output with a simple text message.
> +  This is useful to explain such trivial cases as "No tables used" etc.
> +  
> +  NOTE: Also this function explains the rest of QEP (subqueries if any).

GB68 @note

> +
> +  @param message  text message for the "extra" column.
> +  @param rows     HA_POS_ERROR or a value for the "rows" column.
> +
> +  @returns
> +    @retval FALSE  OK
> +    @retval TRUE   Error
> +*/
> +
> +bool msg_describe(const char *message, ha_rows rows)
> +{
> +  return Explain_msg(message, rows).send();
> +}
> +
> +
> +/**
> +  EXPLAIN handling for single-table UPDATE and DELETE queries
> +
> +  Send to the client a QEP data set for single-table EXPLAIN UPDATE/DELETE
> +  queries. As far as single-table UPDATE/DELETE are implemented without
> +  the regular JOIN tree, we can't reuse mysql_explain_union() directly,
> +  thus we deal with this single table in a special way and then call
> +  mysql_explain_union() for subqueries (if any).
> +
> +  @param table      TABLE object to update/delete rows in the UPDATE/DELETE
> +                    query.
> +  @param select     SQL_SELECT object that represents quick access methods/
> +                    WHERE clause.
> +  @param key        MAX_KEY or and index number of the key that was chosen to
> +                    access table data.
> +  @param limit      HA_POS_ERROR or LIMIT value.
> +  @param need_sort  TRUE if it requires filesort() -- "Using filesort"
> +                    string in the "extra" column.
> +
> +  @returns
> +    @retval FALSE  OK
> +    @retval TRUE   Error
> +*/
> +
> +bool table_describe(TABLE *table, SQL_SELECT *select, uint key, ha_rows limit,
> +                    bool need_sort)
> +{

GB69 I'd add a few DBUG_ENTER/RETURN. Either to the top functions
("table_describe") or to Explain::send(). So that in --debug we at least
have an idea of what's going on.

> +  return Explain_table(table, select, key, limit, need_sort).send();
> +}
> +
> +
> +/**
> +  EXPLAIN handling for EXPLAIN SELECT queries
> +
> +  Send a description about what how the select will be done to the client

GB70 typo

> +
> +  @param join            JOIN
> +  @param need_tmp_table  TRUE if it requires a temporary table --
> +                         "Using temporary" string in the "extra" column.
> +  @param need_order      TRUE if it requires filesort() -- "Using filesort"
> +                         string in the "extra" column.
> +  @param distinct        TRUE if there is the DISTINCT clause (not optimized
> +                         out) -- "Distinct" string in the "extra" column.
> +
> +  @returns
> +    @retval FALSE  OK
> +    @retval TRUE   Error
> +*/
> +
> +bool select_describe(JOIN *join, bool need_tmp_table, bool need_order,
> +                     bool distinct)
> +{
> +  DBUG_ENTER("select_describe");
> +  DBUG_PRINT("info", ("Select 0x%lx, type %s",

GB71 can change 0x%lx to %p

> +		      (ulong)join->select_lex, join->select_lex->type));
> +  if (join->select_lex == join->unit->fake_select_lex)
> +    DBUG_RETURN(Explain_union(join).send());
> +  else
> +    DBUG_RETURN(Explain_join(join, need_tmp_table, need_order, distinct).send());

GB72 the drawback of
f(()
{
   DBUG_RETURN(g());
}
is that in --debug output it looks like
  >f
  <f
  >g
  <g
which does not reflect that f() called g().
I suggest to set a const bool and then DBUG_RETURN it.

> +}
> +
> +
> +/**
> +  EXPLAIN handling for INSERT, REPLACE and multi-table UPDATE/DELETE queries
> +
> +  Send to the client a QEP data set for data-modifying commands those have a
> +  regular JOIN tree (INSERT...SELECT, REPLACE...SELECT and multi-table
> +  UPDATE and DELETE queries) like mysql_select() does for SELECT queries in
> +  the "describe" mode.
> +
> +  NOTE: See table_describe() for single-table UPDATE/DELETE EXPLAIN handling.
> +
> +  NOTE: Unlike the mysql_select() function, explain_data_modification()
> +        calls abort_result_set() itself in the case of failure (OOM etc.)
> +        since explain_data_modification() uses internally created select_result
> +        stream.
> +
> +  @param result  pointer to select_insert, multi_delete or multi_update object:
> +                 the function uses it to call result->prepare(),
> +                 result->prepare2() and result->initialize_tables() only but
> +                 not to modify table data or to send a result to client.
> +  @returns
> +    @retval FALSE  OK
> +    @retval TRUE   Error
> +*/
> +
> +bool explain_data_modification(select_result_interceptor *result)

GB73 I think we need another name.
Even though you explained everything well in comments, if someone looks 
at the function's name (like, he's reading opt_explain.h), he could 
wonder why explain_data_modification() is not called for single-table 
UPDATE/DELETE which are "data modifications". I propose
explain_data_modification -> explain_data_modification_having_JOIN
table_describe -> explain_data_modification_having_no_JOIN
Those names do speak about low-level details (presence or absence of 
JOIN), but it's unavoidable, because it's this (damn) low-level detail 
which forces to have two separate functions.
Even msg_describe() could have a new name: after all, all EXPLAINs send 
messages ("msg")...
Also some functions have "describe" in their name ("table_describe" 
etc), others have "explain" ("explain_data_modification"), could we use 
a single word?
You don't need to change names right now, let's see what the other 
reviewer thinks, he may have better proposals.

> +{
> +  THD *thd= current_thd;

GB74 in places where explain_data_modification() is called, THD is 
known, we can avoid current_thd.

> +  explain_send explain(result);
> +  bool res= thd->send_explain_fields(&explain) ||
> +            mysql_explain_union(thd, &thd->lex->unit, &explain) ||
> +            thd->is_error();
> +  if (res)
> +    explain.abort_result_set();
> +  else
> +    explain.send_eof();
> +  return res;

GB75 this code above resembles what is done in execute_sqlcom_select() 
(if lex->describe is true), which is:

       thd->send_explain_fields(result);
       res= mysql_explain_union(thd, &thd->lex->unit, result);
       ...
       if (res)
         result->abort_result_set();
       else
         result->send_eof();
Can we make execute_sqlcom_select() call explain_data_modification(), to
avoid duplication?
The creation of the warning (of EXPLAIN EXTENDED) can also move into 
explain_data_modification().

> +}
> +
> 
> === added file 'sql/opt_explain.h'
> --- a/sql/opt_explain.h	1970-01-01 00:00:00 +0000
> +++ b/sql/opt_explain.h	2011-04-01 13:53:57 +0000
> @@ -0,0 +1,30 @@
> +/* Copyright (c) 2011, Oracle and/or its affiliates. All rights reserved.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; version 2 of the License.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program; if not, write to the Free Software Foundation,
> +   51 Franklin Street, Suite 500, Boston, MA 02110-1335 USA */
> +
> +
> +#ifndef SQL_EXPLAIN_INCLUDED

GB76 or OPT_EXPLAIN_INCLUDED?

> +#define SQL_EXPLAIN_INCLUDED
> +
> +#include "sql_select.h"

GB77 I don't remember whether the "way to do" nowadays is the include above,
or just forward declarations:
class JOIN;
class TABLE;
etc
#include "my_base.h" // for ha_rows.
sql_select.h is 2280 lines, it describes JOIN... so if I could choose 
I'd prefer to not include it.

> +bool msg_describe(JOIN *join, const char *message);
> +bool msg_describe(const char *message, ha_rows rows= HA_POS_ERROR);
> +bool table_describe(TABLE *table, SQL_SELECT *select, uint key, ha_rows limit,
> +                    bool need_sort);
> +bool select_describe(JOIN *join, bool need_tmp_table, bool need_order,
> +                     bool distinct);
> +bool explain_data_modification(select_result_interceptor *result);

GB78 some "const" can be added over the code; patch is attached.

> +#endif /* SQL_EXPLAIN_INCLUDED */
> 

> === modified file 'sql/sql_class.h'
> --- a/sql/sql_class.h	2011-03-24 08:00:03 +0000
> +++ b/sql/sql_class.h	2011-04-01 13:53:57 +0000
> @@ -3111,6 +3111,14 @@ public:
>    */
>    virtual void cleanup();
>    void set_thd(THD *thd_arg) { thd= thd_arg; }
> +
> +  /*

GB79 /**

> +    If we execute EXPLAIN SELECT ... LIMIT (or any other EXPLAIN query)
> +    we have to ignore LIMIT value sending EXPLAIN output rows since
> +    LIMIT value belongs to the underlying query, not to the whole EXPLAIN.
> +  */
> +  void reset_offset_limit_cnt() { unit->offset_limit_cnt= 0; }
> +
>  #ifdef EMBEDDED_LIBRARY
>    virtual void begin_dataset() {}
>  #else
> 
> === modified file 'sql/sql_delete.cc'
> --- a/sql/sql_delete.cc	2011-03-17 17:39:31 +0000
> +++ b/sql/sql_delete.cc	2011-04-01 13:53:57 +0000
> @@ -35,6 +35,7 @@
>  #include "sp_head.h"
>  #include "sql_trigger.h"
>  #include "transaction.h"
> +#include "opt_explain.h"
>  #include "records.h"                            // init_read_record,
>                                                  // end_read_record
>  
> @@ -60,6 +61,7 @@ bool mysql_delete(THD *thd, TABLE_LIST *
>    ha_rows	deleted= 0;
>    bool          reverse= FALSE;
>    bool          skip_record;
> +  bool          need_sort= FALSE;
>    ORDER *order= (ORDER *) ((order_list && order_list->elements) ?
>                             order_list->first : NULL);
>    uint usable_index= MAX_KEY;
> @@ -144,6 +146,15 @@ bool mysql_delete(THD *thd, TABLE_LIST *
>      /* Update the table->file->stats.records number */
>      table->file->info(HA_STATUS_VARIABLE | HA_STATUS_NO_LOCK);
>      ha_rows const maybe_deleted= table->file->stats.records;
> +
> +    if (thd->lex->describe)
> +    {
> +      bool err= msg_describe("Using delete_all_rows", maybe_deleted);
> +      delete select;
> +      free_underlaid_joins(thd, select_lex);

GB80 same remarks as for sql_update.cc (below), regarding having a 
common exit path.

> +      DBUG_RETURN(err);
> +    }
> +
>      DBUG_PRINT("debug", ("Trying to use delete_all_rows()"));
>      if (!(error=table->file->ha_delete_all_rows()))

GB81 this call may legally fail as "unsupported" (happens in innodb), in
which case we fall back to row-by-row delete. So the msg_describe()
above is premature, it gives false information for an innodb table.

>      {
> @@ -169,14 +180,31 @@ bool mysql_delete(THD *thd, TABLE_LIST *
>      Item::cond_result result;
>      conds= remove_eq_conds(thd, conds, &result);
>      if (result == Item::COND_FALSE)             // Impossible where
> +    {
>        limit= 0;
> +
> +      if (thd->lex->describe)
> +      {
> +        bool err= msg_describe("Impossible WHERE");
> +        delete select;
> +        free_underlaid_joins(thd, select_lex);

GB82 same note about exit paths.

> +        DBUG_RETURN(err);
> +      }
> +    }
>    }
>  
>  #ifdef WITH_PARTITION_STORAGE_ENGINE
>    if (prune_partitions(thd, table, conds))
> -  {
> +  { // No matching records
> +    if (thd->lex->describe)
> +    {
> +      bool err= msg_describe("No matching records");
> +      delete select;
> +      free_underlaid_joins(thd, select_lex);

GB83 same

> +      DBUG_RETURN(err);
> +    }
> +
>      free_underlaid_joins(thd, select_lex);
> -    // No matching record
>      my_ok(thd, 0);
>      DBUG_RETURN(0);
>    }
> @@ -219,23 +247,34 @@ bool mysql_delete(THD *thd, TABLE_LIST *
>        DBUG_RETURN(TRUE);
>      }
>    }
> +
> +  if (order)
> +  {
> +    table->update_const_key_parts(conds);
> +    order= simple_remove_const(order, conds);
> +
> +    usable_index= get_index_for_order(order, table, select, limit,
> +                                      &need_sort, &reverse);
> +  }
> +
> +  if (thd->lex->describe)
> +  {
> +    bool err= table_describe(table, select, usable_index, limit, need_sort);
> +    delete select;
> +    free_underlaid_joins(thd, select_lex);
> +    DBUG_RETURN(err);
> +  }
> +
>    if (options & OPTION_QUICK)
>      (void) table->file->extra(HA_EXTRA_QUICK);

GB84 moving the "quick" down this way is apparently ok. Only MyISAM's
"index page deleting" code makes use of this information, and that code
is called only later in this function.

> -  if (order)
> +  if (need_sort)
>    {
>      uint         length= 0;
>      SORT_FIELD  *sortorder;
>      ha_rows examined_rows;
>      ha_rows found_rows;
>      
> -    table->update_const_key_parts(conds);
> -    order= simple_remove_const(order, conds);
> -
> -    bool need_sort;
> -    usable_index= get_index_for_order(order, table, select, limit,
> -                                      &need_sort, &reverse);
> -    if (need_sort)
>      {
>        DBUG_ASSERT(usable_index == MAX_KEY);
>        table->sort.io_cache= (IO_CACHE *) my_malloc(sizeof(IO_CACHE),
> @@ -361,6 +400,7 @@ bool mysql_delete(THD *thd, TABLE_LIST *
>      (void) table->file->extra(HA_EXTRA_NORMAL);
>  
>  cleanup:
> +  DBUG_ASSERT(!thd->lex->describe);
>    /*
>      Invalidate the table in the query cache if something changed. This must
>      be before binlog writing and ha_autocommit_...
> 
> === modified file 'sql/sql_insert.cc'
> --- a/sql/sql_insert.cc	2011-02-15 17:14:15 +0000
> +++ b/sql/sql_insert.cc	2011-04-01 13:53:57 +0000
> @@ -76,6 +76,7 @@
>  #include "transaction.h"
>  #include "sql_audit.h"
>  #include "debug_sync.h"
> +#include "opt_explain.h"
>  
>  #ifndef EMBEDDED_LIBRARY
>  static bool delayed_get_table(THD *thd, MDL_request *grl_protection_request,
> @@ -770,6 +771,19 @@ bool mysql_insert(THD *thd,TABLE_LIST *t
>    /* Restore the current context. */
>    ctx_state.restore_state(context, table_list);
>  
> +  if (thd->lex->describe)
> +  {
> +    /*
> +      Obviously INSERT without the SELECT is not suitable for EXPLAIN, since we
> +      don't plan how read tables.

GB85 this sentence is strange English, even for a French :-)

> +      So we simply send "No tables used" and stop execution here.

GB86 it might be interesting to tell the user whether the INSERT
will be using bulk insert or not (this can influence speed). But this is
an engine's decision (in its implementation of start_bulk_insert()), and
we usually do not and cannot report in EXPLAIN what the engine decided...

> +    */
> +
> +    bool err= msg_describe("No tables used");

GB87 const bool

> +    free_underlaid_joins(thd, &thd->lex->select_lex);
> +    DBUG_RETURN(err);

GB88 We should try to not add a new exit path. free_underlaid_joins() is
already present in two places in this function, let's not add a third
here, it's too much copy-pasting.
It also has a bug: if executing EXPLAIN INSERT DELAYED, we'll be opening
the table in open_and_lock_for_insert_delayed(), and then we must
release it in end_delayed_insert(); we forget to do it in the current
patch, and so if we
- start mysqld
- type
create table t1 (a int) engine=myisam;
explain insert delayed into t1 values(1);
- send SIGTERM to mysqld
then mysqld hangs, because one thread is in
#1  0x00000000006e3b53 in inline_mysql_mutex_lock (that=0x1406460,
src_file=0xc671c0 "sql_insert.cc", src_line=2714) at
include/mysql/psi/mysql_thread.h:615
#2  0x00000000006ea124 in handle_delayed_insert (arg=0x23608e0) at
sql_insert.cc:2714
because it's waiting for some counter (di->tables_in_use) to be 0, which
doesn't happen as EXPLAIN INSERT DELAYED didn't call end_delayed_insert().
So I would prefer if instead of DBUG_RETURN(err) we would go to the
existing label "abort", which has all we need... except that it returns
TRUE. Maybe we can change it a bit:
- initialize existing variable "error" to 'true'
- rename "abort:" to "exit_without_my_ok:"
- replace all "goto abort" with "goto exit_without_my_ok" and make sure
that error==true at those places
- in code after "exit_without_my_ok:", return "error" instead of TRUE
- in EXPLAIN INSERT, set 'error' to the return value of msg_describe()
and go to "exit_without_my_ok"
?

> +  }
> +
>    /*
>      Fill in the given fields and dump it to the table file
>    */

> === modified file 'sql/sql_update.cc'
> --- a/sql/sql_update.cc	2011-02-25 16:41:57 +0000
> +++ b/sql/sql_update.cc	2011-04-01 13:53:57 +0000
> @@ -1,4 +1,4 @@
> -/* Copyright (c) 2000, 2010, Oracle and/or its affiliates. All rights reserved.
> +/* Copyright (c) 2000, 2011, Oracle and/or its affiliates. All rights reserved.
>  
>     This program is free software; you can redistribute it and/or modify
>     it under the terms of the GNU General Public License as published by
> @@ -38,6 +38,7 @@
>  #include "records.h"                            // init_read_record,
>                                                  // end_read_record
>  #include "filesort.h"                           // filesort
> +#include "opt_explain.h"
>  #include "sql_derived.h" // mysql_derived_prepare,
>                           // mysql_handle_derived,
>                           // mysql_derived_filling
> @@ -392,7 +393,14 @@ int mysql_update(THD *thd,
>  
>  #ifdef WITH_PARTITION_STORAGE_ENGINE
>    if (prune_partitions(thd, table, conds))
> -  {
> +  { // No matching records
> +    if (thd->lex->describe)
> +    {
> +      bool err= msg_describe("No matching records");
> +      free_underlaid_joins(thd, select_lex);
> +      DBUG_RETURN(err);
> +    }

GB89 I'd prefer to avoid adding this call to free_underlaid_joins().
http://dev.mysql.com/doc/refman/5.5/en/explain-output.html
does not mention "No matching records". How about "impossible WHERE"
instead?

> +
>      free_underlaid_joins(thd, select_lex);
>      my_ok(thd);				// No matching records
>      DBUG_RETURN(0);
> @@ -419,6 +427,13 @@ int mysql_update(THD *thd,
>      {
>        DBUG_RETURN(1);				// Error in where
>      }
> +    if (thd->lex->describe)
> +    {
> +      bool err= msg_describe("Impossible WHERE");

GB90 const bool

> +      delete select;
> +      free_underlaid_joins(thd, select_lex);

GB91 "delete" and free_underlaid_joins() have been called in the same 
code branch just a few lines before (line 417), I suspect we mustn't 
repeat that (double "delete" could cause a memory corruption).

> +      DBUG_RETURN(err);
> +    }
>      my_ok(thd);				// No matching records
>      DBUG_RETURN(0);
>    }
> @@ -450,6 +465,15 @@ int mysql_update(THD *thd,
>      used_key_is_modified= is_key_used(table, used_index, table->write_set);
>    }
>  
> +  if (thd->lex->describe)
> +  {
> +    bool err= table_describe(table, select, used_index, limit, 
> +                             order && (need_sort||used_key_is_modified));

GB92 const bool

GB93 'order && (need_sort||used_key_is_modified)' is an expression 
copied from code a bit further down, could we not copy-paste but rather 
store that in some const and use it in the two places?

GB94 there are 3 situations:
a) filesort
b) scan table, collect row ids, update table (it's the "else" branch
after filesort)
c) neither a or b.
In table_describe() above we show "a". Could we also show "b"? Or do we 
already?

> +    delete select;
> +    free_underlaid_joins(thd, select_lex);

GB95 would be great to not add "delete + free_underlaird_joins" again.
I.e. having a common exit path.

> +    DBUG_RETURN(err);
> +  }
> +
>  #ifdef WITH_PARTITION_STORAGE_ENGINE
>    if (used_key_is_modified || order ||
>        partition_key_modified(table, table->write_set))
> @@ -1338,24 +1362,29 @@ bool mysql_multi_update(THD *thd,
>                                (MODE_STRICT_TRANS_TABLES |
>                                 MODE_STRICT_ALL_TABLES));
>  
> -  List<Item> total_list;
> +  if (thd->lex->describe)
> +    res= explain_data_modification(*result);
> +  else
> +  {
> +    List<Item> total_list;
>  
> -  res= mysql_select(thd, &select_lex->ref_pointer_array,
> -                    table_list, select_lex->with_wild,
> -                    total_list,
> -                    conds, 0, (ORDER *) NULL, (ORDER *)NULL, (Item *) NULL,
> -                    (ORDER *)NULL,
> -                    options | SELECT_NO_JOIN_CACHE | SELECT_NO_UNLOCK |
> -                    OPTION_SETUP_TABLES_DONE,
> -                    *result, unit, select_lex);
> -
> -  DBUG_PRINT("info",("res: %d  report_error: %d", res, (int) thd->is_error()));
> -  res|= thd->is_error();
> -  if (unlikely(res))
> -  {
> -    /* If we had a another error reported earlier then this will be ignored */
> -    (*result)->send_error(ER_UNKNOWN_ERROR, ER(ER_UNKNOWN_ERROR));
> -    (*result)->abort_result_set();
> +    res= mysql_select(thd, &select_lex->ref_pointer_array,
> +                      table_list, select_lex->with_wild,
> +                      total_list,
> +                      conds, 0, (ORDER *) NULL, (ORDER *)NULL, (Item *) NULL,
> +                      (ORDER *)NULL,
> +                      options | SELECT_NO_JOIN_CACHE | SELECT_NO_UNLOCK |
> +                      OPTION_SETUP_TABLES_DONE,
> +                      *result, unit, select_lex);
> +
> +    DBUG_PRINT("info",("res: %d  report_error: %d",res, (int) thd->is_error()));
> +    res|= thd->is_error();
> +    if (unlikely(res))
> +    {
> +      /* If we had a another error reported earlier then this will be ignored */
> +      (*result)->send_error(ER_UNKNOWN_ERROR, ER(ER_UNKNOWN_ERROR));
> +      (*result)->abort_result_set();
> +    }
>    }
>    thd->abort_on_warning= 0;
>    DBUG_RETURN(res);
> 
> === modified file 'sql/sql_yacc.yy'
> --- a/sql/sql_yacc.yy	2011-03-09 20:54:55 +0000
> +++ b/sql/sql_yacc.yy	2011-04-01 13:53:57 +0000
> @@ -11376,6 +11376,34 @@ describe:
>              LEX *lex=Lex;
>              lex->select_lex.options|= SELECT_DESCRIBE;
>            }
> +        | describe_command opt_extended_describe
> +          { Lex->describe|= DESCRIBE_NORMAL; }
> +          insert 
> +          {
> +            LEX *lex=Lex;
> +            lex->select_lex.options|= SELECT_DESCRIBE;
> +          }
> +        | describe_command opt_extended_describe
> +          { Lex->describe|= DESCRIBE_NORMAL; }
> +          replace 
> +          {
> +            LEX *lex=Lex;
> +            lex->select_lex.options|= SELECT_DESCRIBE;
> +          }
> +        | describe_command opt_extended_describe
> +          { Lex->describe|= DESCRIBE_NORMAL; }
> +          update 
> +          {
> +            LEX *lex=Lex;
> +            lex->select_lex.options|= SELECT_DESCRIBE;
> +          }
> +        | describe_command opt_extended_describe
> +          { Lex->describe|= DESCRIBE_NORMAL; }
> +          delete 
> +          {
> +            LEX *lex=Lex;
> +            lex->select_lex.options|= SELECT_DESCRIBE;
> +          }
>          ;

GB96 I'm not a parser expert, but isn't it possible to group all those
this way:
         | describe_command opt_extended_describe
           { Lex->describe|= DESCRIBE_NORMAL; }
           explanable_command
           {
             LEX *lex=Lex;
             lex->select_lex.options|= SELECT_DESCRIBE;
           }
explanable_command:
          select | insert | replace | update | delete;
?

>  
>  describe_command:

GB97 as discussed, it would be good to have EXPLAIN EXTENDED print the
transformed query as it does for SELECT.

GB98 testing "EXPLAIN PARTITIONS DELETE/UPDATE/...", without and with
EXTENDED, would be good.

GB99 it would be good to have coverage for all new places where we now
send EXPLAIN output; for example the ones where prune_partitions()
shortcuts DELETE/UPDATE; and others.

GB100 in the WL, there is "The solution is a new explain_send class that
initializes tables like INSERT/REPLACE and multi-table UPDATE/DELETE
do". I suggest to change "INSERT/REPLACE" to "INSERT/REPLACE...SELECT".

GB101 what about unit testing? I'm not sure it is easily doable, given 
the dependency on THD, Field, range optimizer, JOIN, JOIN_TAB...?

GB102 I have to think a bit more about explain_send wrapping the 
interceptor, there may be design changes to suggest there. Also about 
the relationship between mysql_select() and explain_data_modification(). 
Nothing serious though, and not yet clear in my mind. Will keep you posted.

=== modified file 'client/mysqltest.cc'
--- client/mysqltest.cc	2011-03-22 15:40:32 +0000
+++ client/mysqltest.cc	2011-05-09 20:02:28 +0000
@@ -201,6 +201,7 @@
 static my_regex_t ps_re;     /* the query can be run using PS protocol */
 static my_regex_t sp_re;     /* the query can be run as a SP */
 static my_regex_t view_re;   /* the query can be run as a view*/
+static my_regex_t explain_re;/* the query can be converted to EXPLAIN */
 
 static void init_re(void);
 static int match_re(my_regex_t *, char *);
@@ -6389,7 +6390,7 @@
   {"view-protocol", OPT_VIEW_PROTOCOL, "Use views for select.",
    &view_protocol, &view_protocol, 0,
    GET_BOOL, NO_ARG, 0, 0, 0, 0, 0, 0},
-  {"explain-protocol", OPT_EXPLAIN_PROTOCOL, "Explains all select.",
+  {"explain-protocol", OPT_EXPLAIN_PROTOCOL, "Explains all select/update/etc.",
    &explain_protocol, &explain_protocol, 0,
    GET_BOOL, NO_ARG, 0, 0, 0, 0, 0, 0},
   {"connect_timeout", OPT_CONNECT_TIMEOUT,
@@ -7880,7 +7881,7 @@
 {
   if (explain_protocol_enabled &&
       !command->expected_errors.count &&
-      match_re(&view_re, command->query))
+      match_re(&explain_re, command->query))
   {
     st_command save_command= *command;
     DYNAMIC_STRING query_str;
@@ -7890,6 +7891,7 @@
     init_dynamic_string(&query_str, "EXPLAIN EXTENDED ", 256, 256);
     dynstr_append_mem(&query_str, command->query,
                       command->end - command->query);
+    
     command->query= query_str.str;
     command->query_len= query_str.length;
     command->end= strend(command->query);
@@ -7963,9 +7965,16 @@
     "^("
     "[[:space:]]*SELECT[[:space:]])";
 
+
+  /* Filter for queries that can be converted to EXPLAIN */
+  const char *explain_re_str =
+    "^("
+    "[[:space:]]*(SELECT|DELETE|UPDATE|INSERT|REPLACE)[[:space:]])";
+
   init_re_comp(&ps_re, ps_re_str);
   init_re_comp(&sp_re, sp_re_str);
   init_re_comp(&view_re, view_re_str);
+  init_re_comp(&explain_re, explain_re_str);
 }
 
 
@@ -8629,8 +8638,15 @@
 	  strmake(command->require_file, save_file, sizeof(save_file) - 1);
 	  save_file[0]= 0;
 	}
+        /*
+          Gleb:
+          I run EXPLAIN _before_ the query. If query is UPDATE/DELETE is
+          matters: a DELETE may delete rows, and then EXPLAIN DELETE will
+          usually terminate quickly with "no matching rows". To make it more
+          interesting, EXPLAIN is now first.
+        */
+	run_explain(cur_con, command, flags);
 	run_query(cur_con, command, flags);
-	run_explain(cur_con, command, flags);
 	command_executed++;
         command->last_argument= command->end;
 

=== modified file 'sql/opt_explain.cc'
--- sql/opt_explain.cc	2011-03-30 16:48:25 +0000
+++ sql/opt_explain.cc	2011-05-09 20:00:14 +0000
@@ -52,14 +52,14 @@
   Item_float  *col_filtered; ///< "filtered": % of rows filtered by condition
   Item_string *col_extra; ///< "extra" column: additional information
 
-  THD *thd; ///< cached THD pointer
-  const CHARSET_INFO *cs; ///< cached pointer to system_charset_info
-  JOIN *join; ///< top-level JOIN (if any) provided by caller
-  SELECT_LEX *select_lex; ///< cached select_lex pointer
-
-  select_result *external_result; ///< result stream (if any) provided by caller
-
-public:
+  THD * const thd; ///< cached THD pointer
+  const CHARSET_INFO * const cs; ///< cached pointer to system_charset_info
+  JOIN * const join; ///< top-level JOIN (if any) provided by caller
+  SELECT_LEX * const select_lex; ///< cached select_lex pointer
+
+  select_result * const external_result; ///< result stream (if any) provided by
caller
+
+protected:
   explicit Explain(JOIN *join_arg= NULL)
   : nil(NULL),
     thd(current_thd),
@@ -72,6 +72,7 @@
   }
   virtual ~Explain() {}
 
+public:
   bool send();
 
 private:
@@ -80,7 +81,7 @@
   bool push(Item *item) { return items.push_back(item ? item : nil); }
 
 protected:
-  bool describe(uint8 mask) { return thd->lex->describe & mask; }
+  bool describe(uint8 mask) const { return thd->lex->describe & mask; }
 
   /**
     Prepare the self-allocated result object
@@ -192,19 +193,19 @@
 
 class Explain_table_base : public Explain {
 protected:
-  TABLE *table;
+  const TABLE *table;
   key_map usable_keys;
 
   char buff_possible_keys[512];
   String str_possible_keys;
 
-public:
+protected:
   explicit Explain_table_base(JOIN *join_arg)
   : Explain(join_arg), table(NULL),
     str_possible_keys(buff_possible_keys, sizeof(buff_possible_keys), cs)
   {}
 
-  explicit Explain_table_base(TABLE *table_arg)
+  explicit Explain_table_base(const TABLE *table_arg)
   : table(table_arg),
     str_possible_keys(buff_possible_keys, sizeof(buff_possible_keys), cs)
   {}
@@ -286,7 +287,7 @@
 class Explain_table: public Explain_table_base
 {
 private:
-  const SQL_SELECT *select;    ///< cached "select" argument
+  const SQL_SELECT * const select;    ///< cached "select" argument
   const uint       key;        ///< cached "key" number argument
   const ha_rows    limit;      ///< HA_POS_ERROR or cached "limit" argument
   const bool       need_sort;  ///< cached need_sort argument
@@ -302,7 +303,7 @@
   String str_extra;
 
 public:
-  Explain_table(TABLE *table_arg, SQL_SELECT *select_arg,
+  Explain_table(const TABLE *table_arg, const SQL_SELECT *select_arg,
                 uint key_arg, ha_rows limit_arg, bool need_sort_arg)
   : Explain_table_base(table_arg), select(select_arg), key(key_arg),
     limit(limit_arg), need_sort(need_sort_arg),
@@ -679,13 +680,13 @@
   if (table->derived_select_number)
   {
     /* Derived table name generation */
-    int len= my_snprintf(table_name_buffer, sizeof(table_name_buffer) - 1,
-                         "<derived%u>", table->derived_select_number);
+    const int len= my_snprintf(table_name_buffer, sizeof(table_name_buffer) - 1,
+                               "<derived%u>", table->derived_select_number);
     col_table_name= new Item_string(table_name_buffer, len, cs);
   }
   else
   {
-    TABLE_LIST *real_table= table->pos_in_table_list; 
+    const TABLE_LIST *real_table= table->pos_in_table_list;
     col_table_name= new Item_string(real_table->alias,
                                     strlen(real_table->alias), cs);
   }
@@ -708,20 +709,19 @@
   
   if (tab->ref.key_parts)
   {
-    KEY *key_info= table->key_info + tab->ref.key;
-    uint length;
+    const KEY *key_info= table->key_info + tab->ref.key;
     col_key= new Item_string(key_info->name, strlen(key_info->name), cs);
-    length= longlong2str(tab->ref.key_length, buff_key_len, 10) - buff_key_len;
+    const uint length=
+      longlong2str(tab->ref.key_length, buff_key_len, 10) - buff_key_len;
     col_key_len= new Item_string(buff_key_len, length, cs);
     return col_key == NULL || col_key_len == NULL;
   }
   else if (tab->type == JT_NEXT)
   {
-    KEY *key_info= table->key_info + tab->index;
-    uint length;
+    const KEY *key_info= table->key_info + tab->index;
     col_key= new Item_string(key_info->name, strlen(key_info->name), cs);
-    length= longlong2str(key_info->key_length, buff_key_len, 10) - 
-            buff_key_len;
+    const uint length=
+      longlong2str(key_info->key_length, buff_key_len, 10) - buff_key_len;
     col_key_len= new Item_string(buff_key_len, length, cs);
     return col_key == NULL || col_key_len == NULL;
   }
@@ -771,7 +771,12 @@
 
   if (tab->ref.key_parts)
   {
-    for (store_key **ref= tab->ref.key_copy; *ref; ref++)
+    /*
+      Gleb: here I mean that 'ref' is a pointer to a constant pointer to a
+      constant store_key.
+      So *ref=x and **ref=y will be prevented.
+    */
+    for (const store_key * const *ref= tab->ref.key_copy; *ref; ref++)
     {
       if (str_ref.length())
         str_ref.append(',');
@@ -906,7 +911,7 @@
           str_extra.append(STRING_WITH_LEN("; Using where"));
       }
     }
-    TABLE_LIST *table_list= tab->table->pos_in_table_list;
+    const TABLE_LIST *table_list= tab->table->pos_in_table_list;
     if (table_list->schema_table &&
         table_list->schema_table->i_s_requested_object & OPTIMIZE_I_S_TABLE)
     {
@@ -1088,10 +1093,10 @@
 
   if (key != MAX_KEY)
   {
-    KEY *key_info= table->key_info + key;
+    const KEY *key_info= table->key_info + key;
     col_key= new Item_string(key_info->name, strlen(key_info->name), cs);
-    int length= longlong2str(key_info->key_length, buff_key_len, 10) - 
-                             buff_key_len;
+    uint length= longlong2str(key_info->key_length, buff_key_len, 10) -
+      buff_key_len;
     col_key_len= new Item_string(buff_key_len, length, cs);
     return col_key == NULL || col_key_len == NULL;
   }
@@ -1372,7 +1377,7 @@
     @retval TRUE   Error
 */
 
-bool table_describe(TABLE *table, SQL_SELECT *select, uint key, ha_rows limit,
+bool table_describe(const TABLE *table, const SQL_SELECT *select, uint key, ha_rows
limit,
                     bool need_sort)
 {
   return Explain_table(table, select, key, limit, need_sort).send();
@@ -1438,9 +1443,9 @@
 {
   THD *thd= current_thd;
   explain_send explain(result);
-  bool res= thd->send_explain_fields(&explain) ||
-            mysql_explain_union(thd, &thd->lex->unit, &explain) ||
-            thd->is_error();
+  const bool res= thd->send_explain_fields(&explain) ||
+    mysql_explain_union(thd, &thd->lex->unit, &explain) ||
+    thd->is_error();
   if (res)
     explain.abort_result_set();
   else

=== modified file 'sql/opt_explain.h'
--- sql/opt_explain.h	2011-03-30 16:48:25 +0000
+++ sql/opt_explain.h	2011-05-03 20:27:03 +0000
@@ -21,7 +21,7 @@
 
 bool msg_describe(JOIN *join, const char *message);
 bool msg_describe(const char *message, ha_rows rows= HA_POS_ERROR);
-bool table_describe(TABLE *table, SQL_SELECT *select, uint key, ha_rows limit,
+bool table_describe(const TABLE *table, const SQL_SELECT *select, uint key, ha_rows
limit,
                     bool need_sort);
 bool select_describe(JOIN *join, bool need_tmp_table, bool need_order,
                      bool distinct);


Thread
bzr commit into mysql-trunk branch (gleb.shchepa:3358) WL#4897Gleb Shchepa1 Apr
  • Re: bzr commit into mysql-trunk branch (gleb.shchepa:3358) WL#4897Guilhem Bichot28 Apr
    • Re: bzr commit into mysql-trunk branch (gleb.shchepa:3358) WL#4897Gleb Shchepa25 May
      • Re: bzr commit into mysql-trunk branch (gleb.shchepa:3358) WL#4897Guilhem Bichot27 May
  • Re: bzr commit into mysql-trunk branch (gleb.shchepa:3358) WL#4897Guilhem Bichot9 May
    • Re: bzr commit into mysql-trunk branch (gleb.shchepa:3358) WL#4897Gleb Shchepa25 May
      • Re: bzr commit into mysql-trunk branch (gleb.shchepa:3358) WL#4897Guilhem Bichot27 May
        • Re: bzr commit into mysql-trunk branch (gleb.shchepa:3358) WL#4897Gleb Shchepa2 Jun
          • Re: bzr commit into mysql-trunk branch (gleb.shchepa:3358) WL#4897Guilhem Bichot9 Jun
            • Re: bzr commit into mysql-trunk branch (gleb.shchepa:3358) WL#4897Gleb Shchepa9 Jun
    • Re: bzr commit into mysql-trunk branch (gleb.shchepa:3358) WL#4897Roy Lyseng27 May
      • Re: bzr commit into mysql-trunk branch (gleb.shchepa:3358) WL#4897Gleb Shchepa2 Jun
  • Re: bzr commit into mysql-trunk branch (gleb.shchepa:3358) WL#4897Roy Lyseng27 May
    • Re: bzr commit into mysql-trunk branch (gleb.shchepa:3358) WL#4897Gleb Shchepa2 Jun