Re: [Maria-developers] 339dd3d: Invisible Column mysqldump bug fix
Hi, Sachin! Summary: 1. Add a test case, it should include a mix of tables with and without invisible columns, and columns with strange names (reserved words, spaces). 2. Always use full SELECT syntax, even if the table has no invisible columns. On Dec 05, sachin wrote:
revision-id: 339dd3d7508a1e5c7c014b5a59ed99ff0ad96542 (mariadb-10.3.2-59-g339dd3d) parent(s): 6ed3374c8a67ae64448783ab107d1a11cb87f40c author: Sachin Setiya committer: Sachin Setiya timestamp: 2017-12-05 05:35:28 +0530 message:
Invisible Column mysqldump bug fix
--- client/mysqldump.c | 46 +++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 41 insertions(+), 5 deletions(-)
diff --git a/client/mysqldump.c b/client/mysqldump.c index a260065..e59b203 100644 --- a/client/mysqldump.c +++ b/client/mysqldump.c @@ -115,7 +115,7 @@ static my_bool verbose= 0, opt_no_create_info= 0, opt_no_data= 0, opt_no_data_m opt_events= 0, opt_comments_used= 0, opt_alltspcs=0, opt_notspcs= 0, opt_logging, opt_drop_trigger= 0 ; -static my_bool insert_pat_inited= 0, debug_info_flag= 0, debug_check_flag= 0; +static my_bool insert_pat_inited= 0, debug_info_flag= 0, debug_check_flag= 0, invisble_table= 0;
typo, *invisible*
static ulong opt_max_allowed_packet, opt_net_buffer_length; static MYSQL mysql_connection,*mysql=0; static DYNAMIC_STRING insert_pat; @@ -2726,7 +2726,7 @@ static uint get_table_structure(char *table, char *db, char *table_type, complete_insert= 0; if ((write_data= !(*ignore_flag & IGNORE_DATA))) { - complete_insert= opt_complete_insert; + invisble_table= complete_insert= opt_complete_insert; if (!insert_pat_inited) { insert_pat_inited= 1; @@ -2971,6 +2971,12 @@ static uint get_table_structure(char *table, char *db, char *table_type, DBUG_RETURN(0); }
+ while ((row= mysql_fetch_row(result))) + { + if (strlen(row[SHOW_EXTRA]) && strstr(row[SHOW_EXTRA],"INVISIBLE")) + invisble_table= complete_insert= opt_complete_insert= 1;
I think, you need to set invisible_table flag per table, just as you do now, but you should not change complete_insert or opt_complete_insert. That is, if a database has many tables, some with invisible columns and some have only normal columns - tables with invisible columns should use complete_insert, while other tables should not. Basically, INSERT should contain column names if (invisible_table || opt_complete_insert)
+ } + mysql_data_seek(result, 0); /* If write_data is true, then we build up insert statements for the table's data. Note: in subsequent lines of code, this test @@ -3617,7 +3623,7 @@ static void dump_table(char *table, char *db) { char ignore_flag; char buf[200], table_buff[NAME_LEN+3]; - DYNAMIC_STRING query_string; + DYNAMIC_STRING query_string, select_field_names; char table_type[NAME_LEN]; char *result_table, table_buff2[NAME_LEN*2+3], *opt_quoted_table; int error= 0; @@ -3687,7 +3693,25 @@ static void dump_table(char *table, char *db) verbose_msg("-- Sending SELECT query...\n");
init_dynamic_string_checked(&query_string, "", 1024, 1024); + init_dynamic_string_checked(&select_field_names, "", 1024, 1024);
+ if (invisble_table) + { + /* + Copy field name from insert_pat into select_field_names because + select * from table will leave hidden columns. + We will search for "(" in insert_pat and will copy the whole data + into select_field_names , and then we will search for ")" in + select_field_names and decrese the lenght accordingly. + */ + char *ptr= insert_pat.str; + while ( *ptr++ != '(' ){} + dynstr_append_checked(&select_field_names, ptr); + ptr= &select_field_names.str[select_field_names.length - 1]; + while ( *ptr-- != ')') + select_field_names.length--; + select_field_names.str[select_field_names.length- 1]= 0;
please, check how INSERT code is doing it, you need to quote column names.
+ } if (path) { char filename[FN_REFLEN], tmp_path[FN_REFLEN]; @@ -3708,7 +3732,13 @@ static void dump_table(char *table, char *db)
/* now build the query string */
- dynstr_append_checked(&query_string, "SELECT /*!40001 SQL_NO_CACHE */ * INTO OUTFILE '"); + dynstr_append_checked(&query_string, "SELECT /*!40001 SQL_NO_CACHE */ "); + if (invisble_table) + dynstr_append_checked(&query_string, select_field_names.str); + else + dynstr_append_checked(&query_string, "*");
I don't think you need to bother, you can always have a complete column list here. It should not break anything.
+ dynstr_append_checked(&query_string, " INTO OUTFILE '"); + dynstr_free(&select_field_names); dynstr_append_checked(&query_string, filename); dynstr_append_checked(&query_string, "'");
@@ -3757,7 +3787,13 @@ static void dump_table(char *table, char *db) "\n--\n-- Dumping data for table %s\n--\n", fix_for_comment(result_table));
- dynstr_append_checked(&query_string, "SELECT /*!40001 SQL_NO_CACHE */ * FROM "); + dynstr_append_checked(&query_string, "SELECT /*!40001 SQL_NO_CACHE */ "); + if (invisble_table) + dynstr_append_checked(&query_string, select_field_names.str); + else + dynstr_append_checked(&query_string, "*");
same
+ dynstr_free(&select_field_names); + dynstr_append_checked(&query_string, " FROM "); dynstr_append_checked(&query_string, result_table);
if (where) _______________________________________________ commits mailing list commits@mariadb.org https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi Serg! Thanks for the review! On Tue, Dec 5, 2017 at 7:01 PM, Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Sachin!
Summary:
1. Add a test case, it should include a mix of tables with and without invisible columns, and columns with strange names (reserved words, spaces). Done. 2. Always use full SELECT syntax, even if the table has no invisible columns. Done.
On Dec 05, sachin wrote:
revision-id: 339dd3d7508a1e5c7c014b5a59ed99ff0ad96542 (mariadb-10.3.2-59-g339dd3d) parent(s): 6ed3374c8a67ae64448783ab107d1a11cb87f40c author: Sachin Setiya committer: Sachin Setiya timestamp: 2017-12-05 05:35:28 +0530 message:
Invisible Column mysqldump bug fix
--- client/mysqldump.c | 46 +++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 41 insertions(+), 5 deletions(-)
diff --git a/client/mysqldump.c b/client/mysqldump.c index a260065..e59b203 100644 --- a/client/mysqldump.c +++ b/client/mysqldump.c @@ -115,7 +115,7 @@ static my_bool verbose= 0, opt_no_create_info= 0, opt_no_data= 0, opt_no_data_m opt_events= 0, opt_comments_used= 0, opt_alltspcs=0, opt_notspcs= 0, opt_logging, opt_drop_trigger= 0 ; -static my_bool insert_pat_inited= 0, debug_info_flag= 0, debug_check_flag= 0; +static my_bool insert_pat_inited= 0, debug_info_flag= 0, debug_check_flag= 0, invisble_table= 0;
typo, *invisible* Removed.
static ulong opt_max_allowed_packet, opt_net_buffer_length; static MYSQL mysql_connection,*mysql=0; static DYNAMIC_STRING insert_pat; @@ -2726,7 +2726,7 @@ static uint get_table_structure(char *table, char *db, char *table_type, complete_insert= 0; if ((write_data= !(*ignore_flag & IGNORE_DATA))) { - complete_insert= opt_complete_insert; + invisble_table= complete_insert= opt_complete_insert; if (!insert_pat_inited) { insert_pat_inited= 1; @@ -2971,6 +2971,12 @@ static uint get_table_structure(char *table, char *db, char *table_type, DBUG_RETURN(0); }
+ while ((row= mysql_fetch_row(result))) + { + if (strlen(row[SHOW_EXTRA]) && strstr(row[SHOW_EXTRA],"INVISIBLE")) + invisble_table= complete_insert= opt_complete_insert= 1;
I think, you need to set invisible_table flag per table, just as you do now, but you should not change complete_insert or opt_complete_insert.
That is, if a database has many tables, some with invisible columns and some have only normal columns - tables with invisible columns should use complete_insert, while other tables should not. Basically, INSERT should contain column names
Right changed.
if (invisible_table || opt_complete_insert)
+ } + mysql_data_seek(result, 0); /* If write_data is true, then we build up insert statements for the table's data. Note: in subsequent lines of code, this test @@ -3617,7 +3623,7 @@ static void dump_table(char *table, char *db) { char ignore_flag; char buf[200], table_buff[NAME_LEN+3]; - DYNAMIC_STRING query_string; + DYNAMIC_STRING query_string, select_field_names; char table_type[NAME_LEN]; char *result_table, table_buff2[NAME_LEN*2+3], *opt_quoted_table; int error= 0; @@ -3687,7 +3693,25 @@ static void dump_table(char *table, char *db) verbose_msg("-- Sending SELECT query...\n");
init_dynamic_string_checked(&query_string, "", 1024, 1024); + init_dynamic_string_checked(&select_field_names, "", 1024, 1024);
+ if (invisble_table) + { + /* + Copy field name from insert_pat into select_field_names because + select * from table will leave hidden columns. + We will search for "(" in insert_pat and will copy the whole data + into select_field_names , and then we will search for ")" in + select_field_names and decrese the lenght accordingly. + */ + char *ptr= insert_pat.str; + while ( *ptr++ != '(' ){} + dynstr_append_checked(&select_field_names, ptr); + ptr= &select_field_names.str[select_field_names.length - 1]; + while ( *ptr-- != ')') + select_field_names.length--; + select_field_names.str[select_field_names.length- 1]= 0;
please, check how INSERT code is doing it, you need to quote column names.
Done
+ } if (path) { char filename[FN_REFLEN], tmp_path[FN_REFLEN]; @@ -3708,7 +3732,13 @@ static void dump_table(char *table, char *db)
/* now build the query string */
- dynstr_append_checked(&query_string, "SELECT /*!40001 SQL_NO_CACHE */ * INTO OUTFILE '"); + dynstr_append_checked(&query_string, "SELECT /*!40001 SQL_NO_CACHE */ "); + if (invisble_table) + dynstr_append_checked(&query_string, select_field_names.str); + else + dynstr_append_checked(&query_string, "*");
I don't think you need to bother, you can always have a complete column list here. It should not break anything. Done
+ dynstr_append_checked(&query_string, " INTO OUTFILE '"); + dynstr_free(&select_field_names); dynstr_append_checked(&query_string, filename); dynstr_append_checked(&query_string, "'");
@@ -3757,7 +3787,13 @@ static void dump_table(char *table, char *db) "\n--\n-- Dumping data for table %s\n--\n", fix_for_comment(result_table));
- dynstr_append_checked(&query_string, "SELECT /*!40001 SQL_NO_CACHE */ * FROM "); + dynstr_append_checked(&query_string, "SELECT /*!40001 SQL_NO_CACHE */ "); + if (invisble_table) + dynstr_append_checked(&query_string, select_field_names.str); + else + dynstr_append_checked(&query_string, "*");
same Done
+ dynstr_free(&select_field_names); + dynstr_append_checked(&query_string, " FROM "); dynstr_append_checked(&query_string, result_table);
if (where) _______________________________________________ commits mailing list commits@mariadb.org https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits Regards, Sergei Chief Architect MariaDB and security@mariadb.org
_______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
-- Regards Sachin Setiya Software Engineer at MariaDB
participants (2)
-
Sachin Setiya -
Sergei Golubchik