From: Mattias Jonsson
Date: November 18 2010 9:57am
Subject: Re: bzr commit into mysql-5.5-bugteam branch (chris.powers:3128)
 Bug#35333
List-Archive: http://lists.mysql.com/commits/124236
Message-Id: <4CE4F89A.1080106@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Hi Chris,

Patch approved after minor changes, see below.

On 2010-11-18 01.23, Christopher Powers wrote:
> #At file:///home/cpowers/work/dev/mysql-5.5-bugteam/ based on revid:davi.arnaut@stripped
>
>   3128 Christopher Powers	2010-11-17
>        Bug#35333, "If Federated table can't connect to remote host, can't retrieve metadata"
>
>        Improved error handling such that queries against Information_Schema.Tables won't
>        fail if a federated table can't make a remote connection.
>       @ sql/sql_show.cc
>          If get_schema_tables_record() encounters an error, push a warning,
>          set the TABLE COMMENT column with the error text, and clear the
>          error so that the operation can continue.
>
>      added:
>        mysql-test/suite/federated/federated_bug_35333.result
>        mysql-test/suite/federated/federated_bug_35333.test
>      modified:
>        sql/sql_show.cc
> === added file 'mysql-test/suite/federated/federated_bug_35333.result'
> --- a/mysql-test/suite/federated/federated_bug_35333.result	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/federated/federated_bug_35333.result	2010-11-18 00:22:58 +0000
> @@ -0,0 +1,23 @@
> +CREATE DATABASE federated;
> +CREATE DATABASE federated;
> +CREATE DATABASE IF NOT EXISTS realdb;

Why two CREATE federated and one CREATE IF NOT EXISTS realdb ? (be 
consistent). Ah, relates to --source federated.inc, see below.

> +DROP TABLE IF EXISTS realdb.t0;
> +DROP TABLE IF EXISTS federated.t0;
> +CREATE TABLE realdb.t0 (a text, b text) ENGINE=MYISAM;
> +CREATE TABLE federated.t0 (a text, b text) ENGINE=FEDERATED
> +CONNECTION='mysql://root@stripped:3333/realdb/t0';

Hmm, maybe another port? I know that 3333 is not a common port, but I 
think an even higher port, perhaps >= 49152 (see 
http://www.iana.org/assignments/port-numbers), is even more unlikely to 
be used by another service.

> +SELECT TABLE_SCHEMA, TABLE_NAME, TABLE_TYPE, ENGINE, ROW_FORMAT, TABLE_ROWS, DATA_LENGTH, TABLE_COMMENT
> +FROM INFORMATION_SCHEMA.TABLES WHERE TABLE_SCHEMA = 'realdb' or TABLE_SCHEMA = 'federated';
> +TABLE_SCHEMA	TABLE_NAME	TABLE_TYPE	ENGINE	ROW_FORMAT	TABLE_ROWS	DATA_LENGTH	TABLE_COMMENT
> +federated	t0	BASE TABLE	FEDERATED		NULL	0	Unable to connect to foreign data source: Can't connect to MySQL server on '127.0.0.1' (111)
> +realdb	t0	BASE TABLE	MyISAM	Dynamic	0	0	
> +Warnings:
> +Warning	1429	Unable to connect to foreign data source: Can't connect to MySQL server on '127.0.0.1' (111)
> +SHOW WARNINGS;
> +Level	Code	Message
> +Warning	1429	Unable to connect to foreign data source: Can't connect to MySQL server on '127.0.0.1' (111)

Could you do:
let $MYSQLD_DATADIR= `SELECT @@datadir`;
--remove_file $MYSQLD_DATADIR/realdb/t0.MYD
--remove_file $MYSQLD_DATADIR/realdb/t0.MYI
and a new I_S query on the realdb.t0 table? To have a reference to the 
non federated behavior.

> +DROP DATABASE realdb;
> +DROP TABLE IF EXISTS federated.t1;
> +DROP DATABASE federated;
> +DROP TABLE IF EXISTS federated.t1;
> +DROP DATABASE federated;

Double dropping, DROP DATABASE is enough, it should delete all files in 
that directory IIRC. (Ah it comes from a master+slave setup, would have 
been good if it did --echo # on Master + --echo # on Slave first, but 
see below how to fix it.

>
> === added file 'mysql-test/suite/federated/federated_bug_35333.test'
> --- a/mysql-test/suite/federated/federated_bug_35333.test	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/federated/federated_bug_35333.test	2010-11-18 00:22:58 +0000
> @@ -0,0 +1,58 @@
> +# Copyright (c) 2010, 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 */

Not sure if the test really needs the GPL text (at least I never include 
it in any tests).

> +
> +# Test for Federated Engine
> +
> +--source federated.inc

Use --source suite/federated/have_federated_db.inc instead. Will also 
remove the 'CREATE DATABASE federated' and removes the master+slave setup.

> +
> +# Bug 35333 "If a Federated table can't connect to the remote hose, can't retrieve metadata"
> +#
> +# Queries such as SHOW TABLE STATUS and SELECT * FROM INFORMATION_SCHEMA.TABLES fail
> +# when encountering a federated table that cannot connect to its remote table.
> +#
> +# The fix is to store the error text in the TABLE COMMENTS column of I_S.TABLES, clear
> +# the remote connection error and push a warning instead. This allows the SELECT operation
> +# to complete while still indicating a problem.
> +
> +--disable_warnings
> +CREATE DATABASE IF NOT EXISTS realdb;
> +DROP TABLE IF EXISTS realdb.t0;
> +# Federated database already exists

If removing --source federated.inc, add CREATE DATABASE IF NOT EXISTS 
federated.

> +DROP TABLE IF EXISTS federated.t0;
> +--enable_warnings
> +
> +#
> +# Create the base table to be referenced
> +#

You can add '--echo ' before the '#' so it also turns up in the result file.

> +CREATE TABLE realdb.t0 (a text, b text) ENGINE=MYISAM;
> +
> +#
> +# Create a federated table with a bogus port number
> +#
> +CREATE TABLE federated.t0 (a text, b text) ENGINE=FEDERATED
> +    CONNECTION='mysql://root@stripped:3333/realdb/t0';
> +
> +#--warning ER_CONNECT_TO_FOREIGN_DATA_SOURCE
> +
> +SELECT TABLE_SCHEMA, TABLE_NAME, TABLE_TYPE, ENGINE, ROW_FORMAT, TABLE_ROWS, DATA_LENGTH, TABLE_COMMENT
> +       FROM INFORMATION_SCHEMA.TABLES WHERE TABLE_SCHEMA = 'realdb' or TABLE_SCHEMA = 'federated';
> +
> +SHOW WARNINGS;
> +
> +#
> +# Cleanup
> +#
> +DROP DATABASE realdb;
> +source federated_cleanup.inc;

If --source ... have_federated_db.inc is used instead of federated.inc, 
you can replace this with 'DROP DATABASE federated'.

>
> === modified file 'sql/sql_show.cc'
> --- a/sql/sql_show.cc	2010-11-02 14:02:16 +0000
> +++ b/sql/sql_show.cc	2010-11-18 00:22:58 +0000
> @@ -3778,6 +3778,7 @@ static int get_schema_tables_record(THD
>   {
>     const char *tmp_buff;
>     MYSQL_TIME time;
> +   int info_error= 0;
>     CHARSET_INFO *cs= system_charset_info;
>     DBUG_ENTER("get_schema_tables_record");
>
> @@ -3785,20 +3786,16 @@ static int get_schema_tables_record(THD
>     table->field[0]->store(STRING_WITH_LEN("def"), cs);
>     table->field[1]->store(db_name->str, db_name->length, cs);
>     table->field[2]->store(table_name->str, table_name->length, cs);
> +
>     if (res)
>     {
> -    /*
> -      there was errors during opening tables
> -    */
> -    const char *error= thd->is_error() ? thd->stmt_da->message() : "";
> +    /* There was a table open error, so set the table type and fall through */
>       if (tables->view)
>         table->field[3]->store(STRING_WITH_LEN("VIEW"), cs);
>       else if (tables->schema_table)
>         table->field[3]->store(STRING_WITH_LEN("SYSTEM VIEW"), cs);
>       else
>         table->field[3]->store(STRING_WITH_LEN("BASE TABLE"), cs);
> -    table->field[20]->store(error, strlen(error), cs);
> -    thd->clear_error();

Perhaps also add 'goto err;' to prevent future problems if one wants to 
add code before the 'err:' label.

>     }
>     else if (tables->view)
>     {
> @@ -3815,6 +3812,7 @@ static int get_schema_tables_record(THD
>   #ifdef WITH_PARTITION_STORAGE_ENGINE
>       bool is_partitioned= FALSE;
>   #endif
> +
>       if (share->tmp_table == SYSTEM_TMP_TABLE)
>         table->field[3]->store(STRING_WITH_LEN("SYSTEM VIEW"), cs);
>       else if (share->tmp_table)
> @@ -3828,6 +3826,11 @@ static int get_schema_tables_record(THD
>           continue;
>         table->field[i]->set_notnull();
>       }
> +
> +    /*
> +       Collect table info from the table share
> +     */
> +
>   #ifdef WITH_PARTITION_STORAGE_ENGINE
>       if (share->db_type() == partition_hton&&
>           share->partition_info_str_len)
> @@ -3836,62 +3839,82 @@ static int get_schema_tables_record(THD
>         is_partitioned= TRUE;
>       }
>   #endif

Below, not really needed for the patch to add the extra empty lines, but 
is OK by the coding guidelines. So OK to add if it helps readability.

> +
>       tmp_buff= (char *) ha_resolve_storage_engine_name(tmp_db_type);
>       table->field[4]->store(tmp_buff, strlen(tmp_buff), cs);
>       table->field[5]->store((longlong) share->frm_version, TRUE);
>
>       ptr=option_buff;
> +
>       if (share->min_rows)
>       {
>         ptr=strmov(ptr," min_rows=");
>         ptr=longlong10_to_str(share->min_rows,ptr,10);
>       }
> +
>       if (share->max_rows)
>       {
>         ptr=strmov(ptr," max_rows=");
>         ptr=longlong10_to_str(share->max_rows,ptr,10);
>       }
> +
>       if (share->avg_row_length)
>       {
>         ptr=strmov(ptr," avg_row_length=");
>         ptr=longlong10_to_str(share->avg_row_length,ptr,10);
>       }
> +
>       if (share->db_create_options&  HA_OPTION_PACK_KEYS)
>         ptr=strmov(ptr," pack_keys=1");
> +
>       if (share->db_create_options&  HA_OPTION_NO_PACK_KEYS)
>         ptr=strmov(ptr," pack_keys=0");
> +
>       /* We use CHECKSUM, instead of TABLE_CHECKSUM, for backward compability */
>       if (share->db_create_options&  HA_OPTION_CHECKSUM)
>         ptr=strmov(ptr," checksum=1");
> +
>       if (share->db_create_options&  HA_OPTION_DELAY_KEY_WRITE)
>         ptr=strmov(ptr," delay_key_write=1");
> +
>       if (share->row_type != ROW_TYPE_DEFAULT)
>         ptr=strxmov(ptr, " row_format=",
>                     ha_row_type[(uint) share->row_type],
>                     NullS);
> +
>       if (share->key_block_size)
>       {
>         ptr= strmov(ptr, " KEY_BLOCK_SIZE=");
>         ptr= longlong10_to_str(share->key_block_size, ptr, 10);
>       }
> +
>   #ifdef WITH_PARTITION_STORAGE_ENGINE
>       if (is_partitioned)
>         ptr= strmov(ptr, " partitioned");
>   #endif
> +
>       table->field[19]->store(option_buff+1,
>                               (ptr == option_buff ? 0 :
>                                (uint) (ptr-option_buff)-1), cs);
>
>       tmp_buff= (share->table_charset ?
>                  share->table_charset->name : "default");
> +
>       table->field[17]->store(tmp_buff, strlen(tmp_buff), cs);
>
>       if (share->comment.str)
>         table->field[20]->store(share->comment.str, share->comment.length, cs);
>
> +    /*
> +       Collect table info from the storage engine
> +     */
> +

Move the */ one step to the left... Or since it is now OK to use C++ 
comments, use a single line comment, //.

>       if(file)
>       {
> -      file->info(HA_STATUS_VARIABLE | HA_STATUS_TIME | HA_STATUS_AUTO);
xxx12345678901234567890123456789012345678901234567890123456789012345678901234567890
> +      /* If info() returns an error, then there's nothing else to do except return */
> +      if ((info_error= file->info(HA_STATUS_VARIABLE | HA_STATUS_TIME | HA_STATUS_AUTO)) != 0)

To long lines...

> +        goto err;
> +
>         enum row_type row_type = file->get_row_type();
>         switch (row_type) {
>         case ROW_TYPE_NOT_USED:
> @@ -3920,7 +3943,9 @@ static int get_schema_tables_record(THD
>           tmp_buff= "Paged";
>           break;
>         }
> +
>         table->field[6]->store(tmp_buff, strlen(tmp_buff), cs);
> +
>         if (!tables->schema_table)
>         {
>           table->field[7]->store((longlong) file->stats.records, TRUE);
> @@ -3969,6 +3994,26 @@ static int get_schema_tables_record(THD
>         }
>       }
>     }
> +
> +err:
> +  if (res || info_error)
> +  {
> +    /*
> +       If an error was encountered, push a warning, set the TABLE COMMENT
> +       column with the error text, and clear the error so that the operation
> +       can continue.
> +     */

indent the */ one column to the left.

> +    const char *error= thd->is_error() ? thd->stmt_da->message() : "";
> +    table->field[20]->store(error, strlen(error), cs);
> +
> +    if (thd->is_error())
> +    {
> +      push_warning(thd, MYSQL_ERROR::WARN_LEVEL_WARN,
> +                   thd->stmt_da->sql_errno(), thd->stmt_da->message());
> +      thd->clear_error();
> +    }
> +  }
> +
>     DBUG_RETURN(schema_table_store_record(thd, table));
>   }

Regards Mattias