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
