maria-developers team mailing list archive

[Sachin Setiya <sachin.setiya@xxxxxxxxxxx>]

Hi Serg!

Thanks for the review!

On Tue, Dec 5, 2017 at 7:01 PM, Sergei Golubchik <serg@xxxxxxxxxxx> 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@xxxxxxxxxxx
>> https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits
> Regards,
> Sergei
> Chief Architect MariaDB
> and security@xxxxxxxxxxx
>
> _______________________________________________
> Mailing list: https://launchpad.net/~maria-developers
> Post to     : maria-developers@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~maria-developers
> More help   : https://help.launchpad.net/ListHelp



-- 
Regards
Sachin Setiya
Software Engineer at  MariaDB