Hi Chuck,
Charles Bell, 12.11.2009 21:40:
> Ingo,
...
> Can I ask a huge favor? Can you check my logic for my solution to this
> problem? If you'd rather see a working committed patch I'll understand
> and get that to your right away. Bug given that I underestimated the
> complexity in my first attempt another pair of eyes is warranted.
Yes, the new proposal looks good. Please find minor comments below.
...
>> 1) Can we rely on a non-Null engine type in all subpartitions? Probably
>> yes. But then I'd still add an assert, just to be safe.
>
> Do you mean we should assert on p_el->subpartitions? Does the new code
> cover your concern?
Yes, in the new code, no assert is required, as p_el->engine_type is
always checked before use. (I meant to assert on p_el->engine_type, not
p_el->subpartitions, in the former proposal. No such check had been done
after switch to sub_it.)
...
> DIFF for new code:
>
> === modified file 'sql/backup/backup_info.cc'
...
> +/// Check partitions for common storage engine.
Please have a full function comment here.
> +
> +bool partitions_have_same_engine(List_iterator<partition_element> p_it,
> + storage_engine_ref *se_tmp)
> +{
> + partition_element *p_el;
> + while (p_el= p_it++)
Please include the assignment in another parenthesis to avoid compiler
warning.
> + {
> + /*
> + If this partition has subpartitions, we must traverse the
> subpartitions
> + list instead of the partitions list.
> + */
> + if (!p_el->engine_type)
> + {
> + List_iterator<partition_element> p_sub_it(p_el->subpartitions);
> + if (!partitions_have_same_engine(p_sub_it, se_tmp))
> + return FALSE;
Nice trick with the recursive call. :-)
> + }
> + else
> + {
> + if (!*se_tmp)
> + {
> + *se_tmp= hton2plugin[p_el->engine_type->slot];
> + ::handlerton *h= se_hton(*se_tmp);
> +
> + /*
> + Native drivers don't support partitioning. Let Falcon and
> + InnoDB use Snapshot driver; all other storage engines use
> + Default.
> + */
Indentation. (I know it was wrong in old code already.)
> + if (h->start_consistent_snapshot == NULL)
> + return FALSE; // This is not a Falcon or InnoDB storage engine.
> +
> + continue;
> + }
> +
> + // Use Default driver if partitions have different storage engines.
> + if (*se_tmp != hton2plugin[p_el->engine_type->slot])
> + return FALSE;
> + }
> + }
> + return TRUE;
> +}
> +
>
> /// Return storage engine of a given table.
As you do remarkable changes in get_storage_engine(), it would be
appreciated, if you make this a full function comment.
>
> @@ -65,30 +109,8 @@ storage_engine_ref get_storage_engine(TH
> partition_info *p_info= table->part_info;
> List_iterator<partition_element> p_it(p_info->partitions);
> partition_element *p_el;
p_el can be removed too.
> -
> - while ((p_el= p_it++))
> - {
...
> - };
> -
> + if (!partitions_have_same_engine(p_it, &se_tmp))
> + goto close;
> se= se_tmp;
> }
Regards
Ingo
--
Ingo Strüwing, Database Group
Sun Microsystems GmbH, Sonnenallee 1, D-85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schröder, Wolfgang Engels, Wolf Frenkel
Vorsitzender des Aufsichtsrates: Martin Häring HRB München 161028
