Ingo,
Thank you for your review and a very good catch on the memory leak. I added
suggested changes and pushed the updated patch.
Ingo Strüwing wrote:
>> + all_tables= (TABLE_LIST*)my_malloc(tables.count()*sizeof(TABLE_LIST),
> MYF(MY_WME));
>> + DBUG_ASSERT(all_tables); // TODO: report error instead
>> + bzero(all_tables, tables.count()*sizeof(TABLE_LIST));
>
> Suggest my_malloc(..., MYF(MY_WME | MY_ZEROFILL)) instead of bzero().
>
Done.
> And please assure that you will not forget to do the "TODO" later.
>
Right. I leave the code on the same level of error reporting as before my patch.
We have a WL for checking error reporting in all backup code.
>> +
>> + tbl_num++;
>> + cur_table= all_tables[tbl_num - 1].table;
>
> Why not:
> + cur_table= all_tables[tbl_num++].table;
> instead of first incrementing and then subtracting?
>
Indeed, why not :) I've changed this.
>> + for (ulong t=0; t < snap->table_count(); ++t)
>> + {
>> + TABLE_LIST *ptr= (TABLE_LIST*)my_malloc(sizeof(TABLE_LIST), MYF(MY_WME));
>> + DBUG_ASSERT(ptr); // FIXME: report error instead
>> + bzero(ptr, sizeof(TABLE_LIST));
>
> Suggest my_malloc(..., MYF(MY_WME | MY_ZEROFILL)) instead of bzero().
>
I have changed it to alloc_root() so that memory is freed. Thus I need to keep
bzero()...
> And please assure that you will not forget to do the "FIXME" later.
>
>> +
>> + backup::Image_info::Table *tbl= snap->get_table(t);
>> +
>> + ptr->alias= ptr->table_name=
> const_cast<char*>(tbl->name().ptr());
>> + ptr->db= const_cast<char*>(tbl->db().name().ptr());
>
> Is it granted that the life time of tbl->name and tbl->db().name strings
> is longer that that of 'ptr' under all circumstances? (Otherwise we
> would need a strdup here and free the strings when freeing ptr.)
>
Yes, I'm sure that the snap object and the table info stored there live long enough.
>> + ptr->lock_type= TL_WRITE;
>> +
>> + tbl->m_table= ptr;
>
> I didn't find a free() for this in the patch. Did I miss it? I believe I
> saw a warning about this on Saturday already.
>
Good catch! I have switched to using mem_root to avoid this problem.
>> +
>> + ptr->next_global= ptr->next_local=
> ptr->next_name_resolution_table= tables;
>> + tables= ptr;
>
> You diligently build a chain. But the head is an automatic variable and
> thus lost. Do we need a chain then?
>
The chain is only needed for the open_n_lock_tables() call below. After that,
each Table object holds a pointer to the corresponding TABLE_LIST structure.
>> + }
>> + }
>> +
>> + /*
>> + Open and lock the tables.
>> +
>> + Note: simple_open_n_lock_tables() must be used here since we don't want
>> + to do derived tables processing. Processing derived tables even leads
>> + to crashes as those reported in BUG#34758.
>> + */
>> + if (simple_open_n_lock_tables(m_thd,tables))
>> + {
>> + fatal_error(ER_BACKUP_OPEN_TABLES,"RESTORE");
>> + return m_error;
>> + }
Best,
Rafal
