Hi,
Your patch looks good. Patch approved (I vote for the change in
handler.cc, see below).
One review comment is to maybe add something like:
if (!(table1->file->primary_key_is_clustered() &&
table1->s->primary_key
!= MAX_KEY)
#ifdef WITH_PARTITIONING_STORAGE_ENGINE
&& !(table1->part_info)
#endif
)
continue;
In the first level loop to avoid doing the second level loop for tables
not matching the criteria.
To decrease a performance impact when many tables are used (o(n) vs
o(n*n)), but that is optional, since I don't think there will be too
many tables in mysql_multi_update_prepare anyway.
/Mattias
On 2011-02-17 09:55, Jorgen Loland wrote:
> On 02/15/2011 12:45 PM, Jon Olav Hauglid wrote:
>> On 02/15/2011 12:32 PM, Jorgen Loland wrote:
>>>> I think it would be good to also cover the behavior of an InnoDB table
>>>> without a primary key.
>>>
>>> InnoDB always has primary keys. If you don't provide one explicitly,
>>> InnoDB creates one on an autogenerated column. Is this a request
>>> anyway?
>>
>> My thought was just to show that not all multi update statements on an
>> InnoDB table which uses the table twice, are disallowed.
>
> Ok, I will do that in a final patch after Mattias' review.
>
>>
>>>> Is this change really needed anymore?
>>>
>>> No, but I got the impression from Mattias that this was something that
>>> was forgotten. I figured there could be other SQL statements that could
>>> also get into this error code. I don't have a strong opinion either way
>>> - remove or keep?
>>
>> I vote to keep handler.cc unchanged, but I'm open to arguments to the
>> contrary.
>
> Lets see what Mattias thinks :)
>
I vote FOR adding the error code to handler.cc and add a test for
showing this in MyISAM too :) (general errors from storage engines
should be translated to sql-errors if possible, which it is here.)
